On Thu, Jun 16, 2022 2:13 PM Amit Langote <amitlangot...@gmail.com> wrote: > > 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:
Thanks for your comments. > > + * 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. > Modified as you suggested in another mail [1]. > + /* 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. > Modified as suggested. > 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. > Modified as suggested. > 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. > Modified as suggested. Attached the new version of patch set. I also moved the partitioned table check in logicalrep_rel_mark_updatable() to check_relation_updatable() as discussed [2]. [1] https://www.postgresql.org/message-id/CA%2BHiwqG3Xi%3DwH4rBHm61ku-j0gm%2B-rc5VmDHxf%3DTeFkUsHtooA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CA%2BHiwqHfN789ekiYVE%2B0xsLswMosMrWBwv4cPvYgWREWejw7HA%40mail.gmail.com Regards, Shi yu
v10-0001-Fix-partition-table-s-RI-checking-on-the-subscri.patch
Description: v10-0001-Fix-partition-table-s-RI-checking-on-the-subscri.patch
v10-0002-Fix-memory-leak-about-attrmap.patch
Description: v10-0002-Fix-memory-leak-about-attrmap.patch
