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
