On Tue, Jun 14, 2022 2:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:26 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > On Mon, Jun 13, 2022 at 1:03 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > > On Monday, June 13, 2022 1:53 PM Amit Kapila
> <amit.kapila16@gmail.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:
Thanks for your comments.
>
> + 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?
>
I saw some existing tests also use internally generated index name (e.g.
replica_identity.sql, ddl.sql and 031_column_list.pl), so maybe it's better to
fix them all in a separate patch. I didn't change this.
> +# 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.
>
OK, modified.
Attached the new version of patch set, and the patches for pg14 and pg13.
Regards,
Shi yu