On Fri, 21 Nov 2025 at 12:26, Peter Smith <[email protected]> wrote: > > Hi Shlok. > > Here are some review comments for your patch v28-0003 (EXCEPT TABLE ...). > > The review of this patch is a WIP. In this post I only looked at the test > code. > > ====== > .../t/037_rep_changes_except_table.pl > > 1. > + > +# Copyright (c) 2021-2025, PostgreSQL Global Development Group > + > +# Logical replication tests for except table publications > > Use uppercase: /except table/EXCEPT TABLE/ > > ~~~ > > 2. > There are lots of test cases dedicated to partiion-table testing. I > felt a bigger comment separating these major groups might be helpful. > > Something like: > > -- ============================================ > -- EXCEPT TABLE test cases for normal tables > -- ============================================ > > and > > -- ============================================ > -- EXCEPT TABLE test cases for partition tables > -- ============================================ > > ~~~ > > 3. > +# Initialize publisher node > ... > +# Create subscriber node > > Those 2 comments should be almost alike -- e.g. both should say > "Initialize" or both should say "Create". > > ~~~ > > 4. > +# Test replication with publications created using FOR ALL TABLES EXCEPT > TABLE > +# clause. > +# Create schemas and tables on publisher > +$node_publisher->safe_psql( > + 'postgres', qq( > + CREATE SCHEMA sch1; > + CREATE TABLE sch1.tab1 AS SELECT generate_series(1,10) AS a; > + CREATE TABLE public.tab1(a int); > +)); > + > > That first sentence ("Test replication with ...") is not needed here. > The is just repeating the purpose of the entire file, so that comment > can replace the one at the top of this file. > > ~~~ > > 5. > +# Insert some data and verify that inserted data is not replicated > > Be explicit that we are referring to the excluded table. > > SUGGESTION (e.g.) > Verify that data inserted to the excluded table is not replcated. > > ~~~ > > 6. > +# Alter publication to exclude data changes in public.tab1 and verify that > +# subscriber does not get the changed data for this table. > +$node_publisher->safe_psql( > + 'postgres', qq( > + ALTER PUBLICATION tap_pub_schema RESET; > + ALTER PUBLICATION tap_pub_schema ADD ALL TABLES EXCEPT TABLE > (sch1.tab1, public.tab1); > + INSERT INTO public.tab1 VALUES(generate_series(1,10)); > +)); > +$node_publisher->wait_for_catchup('tap_sub_schema'); > + > > It is not strictly needed for these tests, but do you think it makes > more sense to also do an ALTER SUBSCRIPTION ... REFRESH PUBLICATION; > whenever you change the publications? > > ~~~ > > 7. > +# cleanup > +$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_schema"); > +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_schema"); > + > + > > double-blank lines. > > ~~~ > > 8. > I think it would be more helpful if the partition table test cases say > (in their comments) a lot more about the steps they are doing, and > what they expect the result to be. Sure, I can read all the code to > figure it out for each case, but it is better to know the test > intentions/expectations then verify they are doing the right thing. > > ~~~ > > 9. > + CREATE TABLE sch1.t1(a int) PARTITION BY RANGE(a); > + CREATE TABLE sch1.part1 PARTITION OF sch1.t1 FOR VALUES FROM (0) TO (5); > > Maybe create this table to have *multiple* partitions. It might be > interesting later to see what happens when you try to EXCEPT only one > of the partitions. > I have addressed all the comments Please find the updated patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEXwLrQsec6g%2B1dqWTKyJQMQMh%3Dgetj28C%2BzLL14BjuumA%40mail.gmail.com Thanks, Shlok Kyal
