Re: Pgoutput not capturing the generated columns

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.