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
