On Fri, May 29, 2026 at 1:54 PM Peter Smith <[email protected]> wrote:
>
> Hi Nisha.
>
> Some review comments for patch v7-0002.
>
Thanks for the review. All comments are addressed in v8. Please find
responses below for a few of the comments.
> ======
> src/bin/pg_dump/t/002_pg_dump.pl
>
> 2.
> + 'CREATE PUBLICATION pub12' => {
> + create_order => 50,
> + create_sql =>
> + 'CREATE PUBLICATION pub12 FOR TABLES IN SCHEMA dump_test EXCEPT
> (TABLE test_table, dump_test.test_second_table);',
> + regexp => qr/^
> + \QCREATE PUBLICATION pub12 WITH (publish = 'insert, update, delete,
> truncate');\E
> + /xm,
> + like => { %full_runs, section_post_data => 1, },
> + },
> +
> + 'ALTER PUBLICATION pub12 ADD TABLES IN SCHEMA dump_test EXCEPT
> (TABLE test_table, dump_test.test_second_table)'
> + => {
> + regexp => qr/^
> + \QALTER PUBLICATION pub12 ADD TABLES IN SCHEMA dump_test EXCEPT
> (TABLE ONLY test_table, TABLE ONLY test_second_table);\E
> + /xm,
> + like => { %full_runs, section_post_data => 1, },
> + },
>
> I found those hard to read at first. How about just changing the test
> title of the ALTER parts
>
> BEFORE
> + 'ALTER PUBLICATION pub12 ADD TABLES IN SCHEMA dump_test EXCEPT
> (TABLE test_table, dump_test.test_second_table)'
> SUGGESTION
> + 'CREATE PUBLICATION pub12 test continues ...'
>
> (2 places like this)
>
I don't see any existing "..test continues..." pattern, so I changed it as -
'CREATE PUBLICATION pub11 - ADD TABLES IN SCHEMA EXCEPT dump'
Thoughts?
> ======
> src/test/regress/expected/publication.out
>
> 3.
> +-- DROP TABLES IN SCHEMA also removes associated EXCEPT entries
> +ALTER PUBLICATION testpub_alter_except DROP TABLES IN SCHEMA pub_test;
> +\dRp+ testpub_alter_except
> + Publication
> testpub_alter_except
> + Owner | All tables | All sequences | Inserts |
> Updates | Deletes | Truncates | Generated columns | Via root |
> Description
> +--------------------------+------------+---------------+---------+---------+---------+-----------+-------------------+----------+-------------
> + regress_publication_user | f | f | t | t
> | t | t | none | f |
> +Except tables:
> + "pub_test.testpub_tbl_s1"
> +
>
> Isn't this showing a BUG, because after the DROP the "Except tables"
> are still listed.
>
DROP handling is part of Patch-0003, so the DROP-related tests belong
there. I had added the test here only to verify the ADD scenarios, but
I agree that it makes the coverage confusing and incorrect in its
current placement.
I’ve now corrected the tests accordingly.
--
Thanks,
Nisha
