RE: Replica Identity check of partition table on subscriber - Mailing list pgsql-hackers

From shiy.fnst@fujitsu.com
Subject RE: Replica Identity check of partition table on subscriber
Date
Msg-id OSZPR01MB63107255AE453AB182524523FDAA9@OSZPR01MB6310.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Replica Identity check of partition table on subscriber  (Amit Langote <amitlangote09@gmail.com>)
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Masahiko Sawada
Date:
Subject: Re: Skipping logical replication transactions on subscriber side
Next
From: Julien Rouhaud
Date:
Subject: Re: Add header support to text format and matching feature