Re: Replica Identity check of partition table on subscriber

On Mon, Jun 13, 2022 at 9:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Jun 13, 2022 at 1:03 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > On Monday, June 13, 2022 1:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > I have separated out the bug-fix for the subscriber-side.
> > And fix the typo and function name.
> > Attach the new version patch set.
> >
>
> The first patch looks good to me. I have slightly modified one of the
> comments and the commit message. I think we need to backpatch this
> through 13 where we introduced support to replicate into partitioned
> tables (commit f1ac27bf). If you guys are fine, I'll push this once
> the work for PG14.4 is done.

Both the code changes and test cases look good to me. Just a couple
of minor nitpicks with test changes:

+ CREATE UNIQUE INDEX tab5_a_idx ON tab5 (a);
+ ALTER TABLE tab5 REPLICA IDENTITY USING INDEX tab5_a_idx;
+ ALTER TABLE tab5_1 REPLICA IDENTITY USING INDEX tab5_1_a_idx;});

Not sure if we follow it elsewhere, but should we maybe avoid using
the internally generated index name as in the partition's case above?

+# Test the case that target table on subscriber is a partitioned table and
+# check that the changes are replicated correctly after changing the schema of
+# table on subcriber.

The first sentence makes it sound like the tests that follow are the
first ones in the file where the target table is partitioned, which is
not true, so I think we should drop that part. Also how about being
more specific about the test intent, say:

Test that replication continues to work correctly after altering the
partition of a partitioned target table.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com