On Thu, Jun 16, 2022 2:13 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> 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:
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
