Hi, here are my review comments for v8-0001.
======
Commit message.
1.
It seems like the patch name was accidentally omitted, so it became a
mess when it defaulted to the 1st paragraph of the commit message.
======
contrib/test_decoding/test_decoding.c
2.
+ data->include_generated_columns = true;
I previously posted a comment [1, #4] that this should default to
false; IMO it is unintuitive for the test_decoding to have an
*opposite* default behaviour compared to CREATE SUBSCRIPTION.
======
doc/src/sgml/ref/create_subscription.sgml
NITPICK - remove the inconsistent blank line in SGML
======
src/backend/commands/subscriptioncmds.c
3.
+#define SUBOPT_include_generated_columns 0x00010000
I previously posted a comment [1, #6] that this should be UPPERCASE,
but it is not yet fixed.
======
src/bin/psql/describe.c
NITPICK - move and reword the bogus comment
~
4.
+ if (pset.sversion >= 170000)
+ appendPQExpBuffer(&buf,
+ ", subincludegencols AS \"%s\"\n",
+ gettext_noop("include_generated_columns"));
4a.
For consistency with every other parameter, that column title should
be written in words "Include generated columns" (not
"include_generated_columns").
~
4b.
IMO this new column belongs with the other subscription parameter
columns (e.g. put it ahead of the "Conninfo" column).
======
src/test/subscription/t/011_generated.pl
NITPICK - fixed a comment
5.
IMO, it would be better for readability if all the matching CREATE
TABLE for publisher and subscriber are kept together, instead of the
current code which is creating all publisher tables and then creating
all subscriber tables.
~~~
6.
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
+is( $result, qq(4|8
+5|10), 'confirm generated columns ARE replicated when the
subscriber-side column is not generated');
+
...
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
+is( $result, qq(4|24
+5|25), 'confirm generated columns are NOT replicated when the
subscriber-side column is also generated');
+
6a.
These SELECT all need ORDER BY to protect against the SELECT *
returning rows in some unexpected order.
~
6b.
IMO there should be more comments here to explain how you can tell the
column was NOT replicated. E.g. it is because the result value of 'b'
is the subscriber-side computed value (which you made deliberately
different to the publisher-side computed value).
======
99.
Please also refer to the attached nitpicks top-up patch for minor
cosmetic stuff.
======
[1]
https://www.postgresql.org/message-id/CAHv8RjLeZtTeXpFdoY6xCPO41HtuOPMSSZgshVdb%2BV%3Dp2YHL8Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_subscription.sgml
b/doc/src/sgml/ref/create_subscription.sgml
index e8779dc..ee27a58 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -437,7 +437,6 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
associated with the subscription should be replicated.
The default is <literal>false</literal>.
</para>
-
<para>
If the subscriber-side column is also a generated column then this
option
has no effect; the subscriber column will be filled as normal with
the
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 75a52ce..663015d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6609,12 +6609,12 @@ describeSubscriptions(const char *pattern, bool verbose)
appendPQExpBuffer(&buf,
", subskiplsn AS
\"%s\"\n",
gettext_noop("Skip
LSN"));
+
+ /* include_generated_columns is only supported in v18 and
higher */
if (pset.sversion >= 170000)
appendPQExpBuffer(&buf,
",
subincludegencols AS \"%s\"\n",
gettext_noop("include_generated_columns"));
-
- //
include_generated_columns
}
/* Only display subscriptions in current database. */
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 92b3dbf..cbd5015 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -24,7 +24,7 @@ $node_publisher->safe_psql('postgres',
"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a *
2) STORED)"
);
-# publisher-side tab2 has generated col 'b' but subscriber-side tab2 has
NON-gnerated col 'b'.
+# 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 PRIMARY KEY, b int GENERATED ALWAYS AS (a *
2) STORED)"
);
