On Tue, Jun 14, 2022 2:18 PM Amit Langote <amitlangot...@gmail.com> wrote:
> 
> On Mon, Jun 13, 2022 at 9:26 PM Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> > On Mon, Jun 13, 2022 at 1:03 PM houzj.f...@fujitsu.com
> > <houzj.f...@fujitsu.com> wrote:
> > > On Monday, June 13, 2022 1:53 PM Amit Kapila
> <amit.kapil...@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

Attachment: v6-0001-Fix-cache-look-up-failures-while-applying-changes.patch
Description: v6-0001-Fix-cache-look-up-failures-while-applying-changes.patch

Attachment: v6-0002-Reset-partition-map-cache-when-receiving-new-rela.patch
Description: v6-0002-Reset-partition-map-cache-when-receiving-new-rela.patch

Attachment: v6-0003-Check-partition-table-replica-identity-on-subscri.patch
Description: v6-0003-Check-partition-table-replica-identity-on-subscri.patch

Attachment: v6-0004-fix-memory-leak-about-attrmap.patch
Description: v6-0004-fix-memory-leak-about-attrmap.patch

Attachment: v6-pg13-0001-Fix-cache-look-up-failures-while-applying-chang_patch
Description: v6-pg13-0001-Fix-cache-look-up-failures-while-applying-chang_patch

Attachment: v6-pg14-0001-Fix-cache-look-up-failures-while-applying-chang_patch
Description: v6-pg14-0001-Fix-cache-look-up-failures-while-applying-chang_patch

Reply via email to