RE: Replica Identity check of partition table on subscriber

On Thu, Jun 16, 2022 2:13 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Jun 16, 2022 at 2:07 PM shiy(dot)fnst(at)fujitsu(dot)com
> <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> > On Wed, Jun 15, 2022 8:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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