Re: Skipping schema changes in publication - Mailing list pgsql-hackers

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.

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