On Wed, 18 Mar 2026 at 13:35, Peter Smith <[email protected]> wrote:
>
> 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.
Modified the error message
> ======
> 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.
Modified
> 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.
Modified
> 4.
> +CREATE PUBLICATION testpub_forall_tbls_seqs;
>
> Add a comment to say this is creating an "empty" publication in
> preparation for subsequent tests
Modified
> 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
There is an except table testpub_tbl1 already present
> 5b.
> Shouldn't all these tests also all be under the "-- SET ALL
> TABLES/SEQUENCES" test comment?
Modified
> 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?
I felt this comes under permission category, exisiting is ok.
> ======
> 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).
Modified
These comments have been fixed in the v66 version at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm1tKuU479T%3DwinBqoMb3MzO3Mta2juk8W3t2R5ps0_zyg%40mail.gmail.com
Regards,
Vignesh
