Re: Skipping schema changes in publication

Hi Shlok,

Some review comments for patch v17-0003. I also checked the TAP test this time.

======
doc/src/sgml/logical-replication.sgml

1.
+ <literal>publish_generated_columns</literal></link>. Specifying generated
+ columns in a column list using the <literal>EXCEPT</literal> clause excludes
+ the specified generated columns from being published, regardless of the
+ <link linkend="sql-createpublication-params-with-publish-generated-columns">
+ <literal>publish_generated_columns</literal></link> setting. However, for

I think that is not quite the same wording I had previously suggested.
It sounds a bit odd/redundant saying "Specifying" and "specified" in
the same sentence.

======
src/backend/parser/gram.y

2. check_except_collist

I'm wondering if this checking should be done within the existing
preprocess_pubobj_list() function, alongside all the other ERROR
checking. Care needs to be taken to make sure the pubtable->except is
referring to an EXCEPT (col-list), instead of the other kind of EXCEPT
tables, but in general I think it is better to keep all the
publication combinations checking errors like this in one place.

======
src/bin/psql/describe.c

3. addFooterToPublicationDesc

- appendPQExpBuffer(&buf, " (%s)",
- PQgetvalue(result, i, 2));
+ {
+ if (!PQgetisnull(result, i, 3) &&
+ strcmp(PQgetvalue(result, i, 3), "t") == 0)
+ appendPQExpBuffer(&buf, " EXCEPT (%s)",
+ PQgetvalue(result, i, 2));
+ else
+ appendPQExpBuffer(&buf, " (%s)",
+ PQgetvalue(result, i, 2));
+ }

Do you really need to check !PQgetisnull(result, i, 3) here? (e.g.
The comment does not say that this attribute can be NULL)

======
.../t/037_rep_changes_except_collist.pl

4.
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+# Logical replication tests for except table publications

Comment is wrong. These tests are for EXCEPT (column-list)

~~~

5.
+# Test for except column publications
+# Initial setup
+$node_publisher->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab2 (a int, b int NOT NULL, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE sch1.tab2 (a int, b int, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab3 (a int, b int, c int)");
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab4 (a int, b int GENERATED ALWAYS AS (a * 2) STORED,
c int GENERATED ALWAYS AS (a * 3) STORED)"
+);
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab5 (a int, b int GENERATED ALWAYS AS (a * 2) STORED,
c int GENERATED ALWAYS AS (a * 3) STORED)"
+);
+$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (1, 2, 3)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab2 VALUES (1, 2, 3)");
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub_col FOR TABLE tab2 EXCEPT (a), sch1.tab2
EXCEPT (b, c)"
+);

5a.
I think you don't need to say "Test for except column publications",
because that is the purpose of thie entire file.

~

5b.
You can combine multiple of these safe_psql calls together

~

5c.
It might help make tests easier to read if you named those generated
columns 'b', 'c' cols as 'bgen', 'cgen' instead.

~

5d.
The table names are strange, because why does it start at tab2 when
there is not a tab1?

~~~

6.
+$node_subscriber->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab2 (a int, b int NOT NULL, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE sch1.tab2 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab3 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab4 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab5 (a int, b int, c int)");

You can combine multiple of these safe_psql calls together

~~~

7.
+# Test initial sync
+my $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
+is($result, qq(|2|3),
+ 'check that initial sync for except column publication');

The message seems strange. Do you mean "check initial sync for an
'EXCEPT (column-list)' publication"

NOTE: There are many other messages where you wrote "for except column
publication" but I think maybe all of those can be improved a bit like
above.

~~~

8.
+$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (4, 5, 6)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab2 VALUES (4, 5, 6)");
+$node_publisher->wait_for_catchup('tap_sub_col');

8a.
You can combine multiple of these safe_psql calls together.

NOTE: I won't keep repeating this review comment but I think maybe
there are lots more places where the safe_psql can all be combined to
expected multiple statements.

~

8b.
I felt all those commands should be under the "Test incremental
changes" comment.

~~~

9.
+is($result, qq(1||3), 'check alter publication with EXCEPT');

Maybe that should've said with 'EXCEPT (column-list)'

~~~

10.
+# Test for publication created with publish_generated_columns as true on table
+# with generated columns and column list specified with EXCEPT
+$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (1)");
+$node_publisher->safe_psql('postgres',
+ "ALTER PUBLICATION tap_pub_col SET (publish_generated_columns)");
+$node_publisher->safe_psql('postgres',
+ "ALTER PUBLICATION tap_pub_col SET TABLE tab4 EXCEPT(b)");
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub_col REFRESH PUBLICATION");
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub_col');

10a.
I felt the test comments for both those generated columns parameter
test should give more explanation to say what is the expected result
and why.

~

10b.
How does "ALTER PUBLICATION tap_pub_col SET
(publish_generated_columns)" even work? I thought the
"pubish_generated_columns" is an enum but you did not specify any enum
value here (???)

~~~

11.
+ 'check publication(publish_generated_columns as false) with
generated columns and EXCEPT'

Hmm. I thought there is no such thing as "publish_generated_columns as
false", and also the EXCEPT should say 'EXCEPT (column-list)'

~~~

12.
I wonder if there should be another boundary condition test case as follows:
- have some table with cols a,b,c.
- create a publication 'EXCEPT (a,b,c)', so you don't publish anything at all.
- then ALTER the TABLE to add a column 'd'.
- now the publication should publish only 'd'.

======

Kind Regards,
Peter Smith.
Fujitsu Australia