Hi, On Thu, Jun 16, 2022 at 2:07 PM shiy.f...@fujitsu.com <shiy.f...@fujitsu.com> wrote: > On Wed, Jun 15, 2022 8:30 PM Amit Kapila <amit.kapil...@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
