Hi, here are some patch v11-0001 comments. (BTW, I had difficulty reviewing this because something seemed strange with the changes this patch made to the test_decoding tests).
======
General
1. Patch name
Patch name does not need to quote 'logical replication'
~
2. test_decoding tests
Multiple test_decoding tests were failing for me. There is something
very suspicious about the unexplained changes the patch made to the
expected "binary.out" and "decoding_into_rel.out" etc. I REVERTED all
those changes in my nitpicks top-up to get everything working. Please
re-confirm that all the test_decoding tests are OK!
======
Commit Message
3.
Since you are including the example usage for test_decoding, I think
it's better to include the example usage of CREATE SUBSCRIPTION also.
======
contrib/test_decoding/expected/binary.out
4.
SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot',
'test_decoding');
- ?column?
-----------
- init
-(1 row)
-
+ERROR: replication slot "regression_slot" already exists
Huh? Why is this unrelated expected output changed by this patch?
The test_decoding test fails for me unless I REVERT this change!! See
my nitpicks diff.
======
.../expected/decoding_into_rel.out
5.
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
- ?column?
-----------
- stop
-(1 row)
-
Huh? Why is this unrelated expected output changed by this patch?
The test_decoding test fails for me unless I REVERT this change!! See
my nitpicks diff.
======
.../test_decoding/sql/decoding_into_rel.sql
6.
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
Huh, Why does this patch change this code at all? I REVERTED this
change. See my nitpicks diff.
======
.../test_decoding/sql/generated_columns.sql
(see my nitpicks replacement file for this test)
7.
+-- test that we can insert the result of a 'include_generated_columns'
+-- into the tables created. That's really not a good idea in practical terms,
+-- but provides a nice test.
NITPICK - I didn't understand the point of this comment. I updated
the comment according to my understanding.
~
NITPICK - The comment "when 'include-generated-columns' is not set
then columns will not be replicated" is the opposite of what the
result is. I changed this comment.
NITPICK - modified and unified wording of some of the other comments
NITPICK - changed some blank lines
======
contrib/test_decoding/test_decoding.c
8.
+ else if (strcmp(elem->defname, "include-generated-columns") == 0)
+ {
+ if (elem->arg == NULL)
+ data->include_generated_columns = true;
Is there any way to test that "elem->arg == NULL" in the
generated.sql? OTOH, if it is not possible to get here then is the
code even needed?
======
doc/src/sgml/ddl.sgml
9.
<para>
- Generated columns are skipped for logical replication and cannot be
- specified in a <command>CREATE PUBLICATION</command> column list.
+ 'include_generated_columns' option controls whether generated columns
+ should be included in the string representation of tuples during
+ logical decoding in PostgreSQL. The default is <literal>true</literal>.
</para>
NITPICK - Use proper markdown instead of single quotes for the parameter.
NITPICK - I think this can be reworded slightly to provide a
cross-reference to the CREATE SUBSCRIPTION parameter for more details
(which means then we can avoid repeating details like the default
value here). PSA my nitpicks diff for an example of how I thought docs
should look.
======
doc/src/sgml/protocol.sgml
10.
+ The default is true.
No, it isn't. AFAIK you made the default behaviour true only for
'test_decoding', but the default for CREATE SUBSCRIPTION remains
*false* because that is the existing PG17 behaviour. And the default
for the 'include_generated_columns' in the protocol is *also* false to
match the CREATE SUBSCRIPTION default.
e.g. libpqwalreceiver.c only sets ", include_generated_columns 'true'"
when options->proto.logical.include_generated_columns
e.g. worker.c says: options->proto.logical.include_generated_columns =
MySubscription->includegencols;
e.g. subscriptioncmds.c sets default: opts->include_generated_columns = false;
(This confirmed my previous review expectation that using different
default behaviours for test_decoding and pgoutput would surely lead to
confusion)
~~~
11.
- <para>
- Next, the following message part appears for each column included in
- the publication (except generated columns):
- </para>
-
AFAIK you cannot just remove this entire paragraph because I thought
it was still relevant to talking about "... the following message
part". But, if you don't want to explain and cross-reference about
'include_generated_columns' then maybe it is OK just to remove the
"(except generated columns)" part?
======
src/test/subscription/t/011_generated.pl
NITPICK - comment typo /tab2/tab3/
NITPICK - remove some blank lines
~~~
12.
# the column was NOT replicated (the result value of 'b' is the
subscriber-side computed value)
NITPICK - I think this comment is wrong for the tab2 test because here
col 'b' IS replicated. I have added much more substantial test case
comments in the attached nitpicks diff. PSA.
======
src/test/subscription/t/031_column_list.pl
13.
NITPICK - IMO there is a missing word "list" in the comment. This bug
existed already on HEAD but since this patch is modifying this comment
I think we can also fix this in passing.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
diff --git a/contrib/test_decoding/expected/binary.out
b/contrib/test_decoding/expected/binary.out
index c30abc7..b3a3509 100644
--- a/contrib/test_decoding/expected/binary.out
+++ b/contrib/test_decoding/expected/binary.out
@@ -1,7 +1,11 @@
-- predictability
SET synchronous_commit = on;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
-ERROR: replication slot "regression_slot" already exists
+ ?column?
+----------
+ init
+(1 row)
+
-- succeeds, textual plugin, textual consumer
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'force-binary', '0', 'skip-empty-xacts', '1');
data
diff --git a/contrib/test_decoding/expected/decoding_into_rel.out
b/contrib/test_decoding/expected/decoding_into_rel.out
index f763e05..8fd3390 100644
--- a/contrib/test_decoding/expected/decoding_into_rel.out
+++ b/contrib/test_decoding/expected/decoding_into_rel.out
@@ -103,3 +103,9 @@ SELECT data FROM
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
COMMIT
(14 rows)
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
+ ?column?
+----------
+ stop
+(1 row)
+
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 0f62013..91a292b 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -514,9 +514,10 @@ CREATE TABLE people (
</listitem>
<listitem>
<para>
- 'include_generated_columns' option controls whether generated columns
- should be included in the string representation of tuples during
- logical decoding in PostgreSQL. The default is <literal>true</literal>.
+ Generated columns may be skipped during logical replication according to
the
+ <command>CREATE SUBSCRIPTION</command> option
+ <link
linkend="sql-createsubscription-params-with-include-generated-columns">
+ <literal>include_generated_columns</literal></link>,
</para>
</listitem>
</itemizedlist>
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index bc6033a..25edc6f 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -31,13 +31,11 @@ $node_subscriber->safe_psql('postgres',
# publisher-side tab2 has generated col 'b' but subscriber-side tab2 has
NON-generated col 'b'.
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab2 (a int, b int GENERATED ALWAYS AS (a * 2) STORED)");
-
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab2 (a int, b int)");
-# publisher-side tab3 has generated col 'b' but subscriber-side tab2 has
DIFFERENT COMPUTATION generated col 'b'.
+# publisher-side tab3 has generated col 'b' but subscriber-side tab3 has
DIFFERENT COMPUTATION generated col 'b'.
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 10) STORED)");
-
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 20) STORED)");
@@ -60,11 +58,9 @@ $node_publisher->safe_psql('postgres',
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION
pub1"
);
-
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION sub2 CONNECTION '$publisher_connstr' PUBLICATION
pub2 WITH (include_generated_columns = true, copy_data = false)"
);
-
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION sub3 CONNECTION '$publisher_connstr' PUBLICATION
pub3 WITH (include_generated_columns = true, copy_data = false)"
);
@@ -98,11 +94,12 @@ is( $result, qq(1|22|
4|88|
6|132|), 'generated columns replicated');
+#
+# TEST tab2: the publisher-side col 'b' is generated, and the subscriber-side
+# col 'b' is not generated, so confirm that col 'b' IS replicated.
+#
$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (4), (5)");
-
$node_publisher->wait_for_catchup('sub2');
-
-# the column was NOT replicated (the result value of 'b' is the
subscriber-side computed value)
$result =
$node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab2 ORDER BY a");
is( $result, qq(4|8
@@ -110,10 +107,14 @@ is( $result, qq(4|8
'confirm generated columns ARE replicated when the subscriber-side
column is not generated'
);
+#
+# TEST tab3: the publisher-side col 'b' is generated, and the subscriber-side
+# col 'b' is also generated, so confirmed that col 'b' IS NOT replicated. We
+# can know this because the result value is the subscriber-side computation
+# (which is not the same as the publisher-side computation for col 'b').
+#
$node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
-
$node_publisher->wait_for_catchup('sub3');
-
$result =
$node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab3 ORDER BY a");
is( $result, qq(4|24
diff --git a/src/test/subscription/t/031_column_list.pl
b/src/test/subscription/t/031_column_list.pl
index 6e73f89..9804158 100644
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -1204,7 +1204,7 @@ t), 'check the number of columns in the old tuple');
# TEST: Dropped columns are not considered for the column list.
# So, the publication having a column list except for those columns and a
-# publication without any column (aka all columns as part of the columns
+# publication without any column list (aka all columns as part of the columns
# list) are considered to have the same column list.
$node_publisher->safe_psql(
'postgres', qq(
PS_NITPICKS_20240627_GENCOLS_V110001_generated_columns.sql
Description: Binary data
