Horton

Re: Replica Identity check of partition table on subscriber

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Date: 2022-06-16T12:28:40Z
Lists: pgsql-hackers 
On Thu, Jun 16, 2022 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Jun 16, 2022 at 6:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > @@ -1735,6 +1735,13 @@ apply_handle_insert_internal(ApplyExecutionData *edata,
> > >  static void
> > >  check_relation_updatable(LogicalRepRelMapEntry *rel)
> > >  {
> > > +   /*
> > > +    * If it is a partitioned table, we don't check it, we will check its
> > > +    * partition later.
> > > +    */
> > > +   if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> > > +       return;
> > >
> > > Why do this?  I mean why if logicalrep_check_updatable() doesn't care
> > > if the relation is partitioned or not -- it does all the work
> > > regardless.
> > >
> > > I suggest we don't add this check in check_relation_updatable().
> >
> > I think based on this suggestion patch has moved this check to
> > logicalrep_rel_mark_updatable(). For a partitioned table, it won't
> > even validate whether it can mark updatable as false which seems odd
> > to me even though there might not be any bug due to that. Was your
> > suggestion actually intended to move it to
> > logicalrep_rel_mark_updatable?
>
> No, I didn't intend to suggest that we move this check to
> logicalrep_rel_mark_updatable(); didn't notice that that's what the
> latest patch did.
>
> What I said is that we shouldn't ignore the updatable flag for a
> partitioned table in check_relation_updatable(), because
> logicalrep_rel_mark_updatable() would have set the updatable flag
> correctly even for partitioned tables.  IOW, we should not
> special-case partitioned tables anywhere.
>
> I guess the point of adding the check is to allow the case where a
> leaf partition's replica identity can be used to apply an update
> originally targeting its ancestor that doesn't itself have one.
>
> I wonder if it wouldn't be better to move the
> check_relation_updatable() call to
> apply_handle_{update|delete}_internal()?  We know for sure that we
> only ever get there for leaf tables.  If we do that, we won't need the
> relkind check.
>

I think this won't work for updates via apply_handle_tuple_routing()
unless we call it from some other place(s) as well. It will do
FindReplTupleInLocalRel() before doing update/delete for CMD_UPDATE
case and will lead to assertion failure.

-- 
With Regards,
Amit Kapila.

Commits

  1. Fix memory leak due to LogicalRepRelMapEntry.attrmap.

  2. Fix stale values in partition map entries on subscribers.

  3. Fix partition table's REPLICA IDENTITY checking on the subscriber.

  4. Fix data inconsistency between publisher and subscriber.

  5. Fix cache look-up failures while applying changes in logical replication.

  6. Add logical replication support to replicate into partitioned tables