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