Horton

Re: Replica Identity check of partition table on subscriber

Amit Langote <amitlangote09@gmail.com>

View thread View on postgr.es
From: Amit Langote <amitlangote09@gmail.com>
To: Amit Kapila <amit.kapila16@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-16T11:54:41Z
Lists: pgsql-hackers 
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.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

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