RE: Added schema level support for publication. - Mailing list pgsql-hackers

On Monday, July 26, 2021 1:25 PM vignesh C <vignesh21@gmail.com> wrote:

> 

> On Mon, Jul 26, 2021 at 7:41 AM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote:

> >

> > On Friday, July 23, 2021 8:18 PM vignesh C <vignesh21@gmail.com> wrote:

> > >

> > > I have changed it to not report any error, this issue is fixed in the

> > > v14 patch attached at [1].

> > > [1] - https://www.postgresql.org/message-

> > > id/CALDaNm3DTj535ezfmm8QHLOtOkcHF2ZcCfSjfR%3DVbTbLZXFRsA%40mail.g

> > > mail.com

> > >

> >

> > Thanks for your new patch. But there's a conflict when apply patch v14 on the latest HEAD (it seems caused by commit #678f5448c2d869), please rebase it.

> >

>

> Thanks for reporting it, it is rebased at the v15 patch attached at [1].

> [1] - https://www.postgresql.org/message-id/CALDaNm27LRWF9ney%3DcVeD-0jc2%2BJ5Y0wNQhighZB%3DAat4VbNBA%40mail.gmail.com

Thanks for your new patch. I applied your patch and it succeeded.

Here are some comments on the tests in your patch. The attached file included changes about the comments, please have a look.

1.  src/test/regress/sql/publication.sql

There are some existing tests to verify that we can't add table to ‘for all tables publication', should we add some tests about adding schema to ‘for all tables publication’ / ‘for table publication’?

I added some cases in the attached file.

Besides, the following existing comment seems not suitable. It uses 'SET' but the comment says 'add'. And since we can add table or schema, I think we should point out what we add is table, thoughts?

-- fail - can't add to for all tables publication

ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk;

2. src/test/subscription/t/001_rep_changes.pl

2.1

+# Insert some data into few tables and verify that inserted data is replicated.

+$node_publisher->safe_psql('postgres', "INSERT INTO  sch1.tab1 VALUES(generate_series(11,20))");

+$node_publisher->safe_psql('postgres', "INSERT INTO  sch2.tab1 VALUES(generate_series(11,20))");

+

+$node_publisher->wait_for_catchup('tap_sub_schema');

There are two spaces between "INTO" and "sch1.tab1".

2.2

Most of publication names are lowercase, and some of them are uppercase. I think it will be better if all of them are lowercase.

I modified them in the attached file.

2.3

+# Verify that the subscription relation list is updated after refresh.

+$result = $node_subscriber->safe_psql('postgres',

+        "SELECT count(*) FROM pg_subscription_rel WHERE srsubid IN (SELECT oid FROM pg_subscription WHERE subname = 'tap_sub_schema')");

+is($result, qq(5),

+        'check subscription relation status was dropped on subscriber');

Should it be 'check subscription relation status is not yet dropped on subscriber' here?

2.4

+# Drop publications as we don't need them anymore

+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_schema");

There is only one publication, so the comment here should be 'Drop publication as we don't need it anymore'.

3.

There are some existing test cases about publication for table and publication for all tables in 002_pg_dump.pl, so I think we could add some test cases about publication for schema.

I tried to add some cases in the attached file.

Regards

Tang