Re: Added schema level support for publication.

On Tue, Jul 27, 2021 at 12:21 PM tanghy(dot)fnst(at)fujitsu(dot)com
<tanghy(dot)fnst(at)fujitsu(dot)com> wrote
>
> 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.

I have taken the changes.

>
> 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;
>

Since this is in base code, you might want to post a patch on a separate thread.

> 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".

I have taken the changes.

> 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.

I have taken the changes.

> 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?

I have taken the changes.

> 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'.

I have taken the changes.

> 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.

Thanks for the patch, I have merged the changes. Attached v16 patch
has the fixes for the same.

Regards,
Vignesh