Re: Skipping schema changes in publication

Hi Vignesh

Here are some review comments for v65-0001 (test code part)

======
src/test/regress/expected/publication.out

1.
+-- fail - SET ALL TABLES on a publication requires superuser privileges
+ALTER PUBLICATION testpub5 SET ALL TABLES EXCEPT TABLE (testpub_tbl1); -- fail
+ERROR:  must be superuser to alter FOR ALL TABLES publication
+ALTER PUBLICATION testpub5 SET ALL TABLES; -- fail
+ERROR:  must be superuser to alter FOR ALL TABLES publication
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4, testpub5;

The error messages don't quite seem correct. e.g. You are not trying
"to alter FOR ALL TABLES publication". You are trying to alter a
publication to convert it to a  "FOR ALL TABLES" publication.

======
src/test/regress/sql/publication.sql

2.
+-- fail - SET ALL TABLES is not allowed for a 'FOR TABLE' publication

I think these failure tests should be under the "-- SET ALL
TABLES/SEQUENCES" test comment.

~~~

3.
+-- fail - SET ALL TABLES is not allowed for a schema publication

I think these failure tests should be under the "-- SET ALL
TABLES/SEQUENCES" test comment.

~~~

4.
+CREATE PUBLICATION testpub_forall_tbls_seqs;

Add a comment to say this is creating an "empty" publication in
preparation for subsequent tests

~~~

5.
+-- Remove all the EXCEPT tables.
+ALTER PUBLICATION testpub_foralltables_excepttable SET ALL TABLES;
+\dRp+ testpub_foralltables_excepttable
+
+-- Replace the publication EXCEPT table list with a specific EXCEPT table.
+ALTER PUBLICATION testpub_foralltables_excepttable SET ALL TABLES
EXCEPT TABLE (testpub_tbl1);
+\dRp+ testpub_foralltables_excepttable

5a.
Are these test OK?
* The 1st test says "Remove EXCEPT" but was there any EXCEPT list to start with?
* The 2nd test say "Replace EXCEPT" but there was nothing replace
because previous test removed it.
* e.g. there seems not test that replaces one EXCEPT with a different EXCEPT

~

5b.
Shouldn't all these tests also all be under the "-- SET ALL
TABLES/SEQUENCES" test comment?

~~~

6.
+-- fail - SET ALL TABLES on a publication requires superuser privileges
+ALTER PUBLICATION testpub5 SET ALL TABLES EXCEPT TABLE (testpub_tbl1); -- fail
+ALTER PUBLICATION testpub5 SET ALL TABLES; -- fail

Shouldn't all these tests also all be under the "-- SET ALL
TABLES/SEQUENCES" test comment?

======
src/test/subscription/t/037_except.pl

7.
+# Verify that table synchronization occurs once tab1 is removed from the
+# EXCEPT TABLE clause via SET ALL TABLES EXCEPT TABLE.
+$result =
+  $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab1");
+is($result, qq(20),
+ 'check that the data is copied as the tab1 is removed from EXCEPT
TABLE clause'
+);

The comment is strangely worded. Talking about removing from EXCEPT
lists is confusing. How about?

-- Verify that table synchronization now happens for tab1.
-- tab1 is included now since the EXCEPT TABLE list is only (tab2).

======
Kind Regards,
Peter Smith.
Fujitsu Australia