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
