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: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Date: 2022-06-16T06:13:07Z
Lists: pgsql-hackers 
Hi,

On Thu, Jun 16, 2022 at 2:07 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
> On Wed, Jun 15, 2022 8:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I have pushed the first bug-fix patch today.
>
> Attached the remaining patches which are rebased.

Thanks.

Comments on v9-0001:

+ * Don't throw any error here just mark the relation entry as not updatable,
+ * as replica identity is only for updates and deletes but inserts can be
+ * replicated even without it.

I know you're simply copying the old comment, but I think we can
rewrite it to be slightly more useful:

We just mark the relation entry as not updatable here if the local
replica identity is found to be insufficient and leave it to
check_relation_updatable() to throw the actual error if needed.

+   /* Check that replica identity matches. */
+   logicalrep_rel_mark_updatable(entry);

Maybe the comment (there are 2 instances) should say:

Set if the table's replica identity is enough to apply update/delete.

Finally,

+# Alter REPLICA IDENTITY on subscriber.
+# No REPLICA IDENTITY in the partitioned table on subscriber, but what we check
+# is the partition, so it works fine.

For consistency with other recently added comments, I'd suggest the
following wording:

Test that replication works correctly as long as the leaf partition
has the necessary REPLICA IDENTITY, even though the actual target
partitioned table does not.

On v9-0002:

+   /* cleanup the invalid attrmap */

It seems that "invalid" here really means no-longer-useful, so we
should use that phrase as a nearby comment does:

Release the no-longer-useful attrmap, if any.

-- 
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