Re: Pgoutput not capturing the generated columns

On Thu, Jun 27, 2024 at 2:41 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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?
>

Currently I could not find a case where the
'include_generated_columns' option is not specifying any value, but  I
was hesitant to remove this from here as the other options mentioned
follow the same rules. Thoughts?

> ======
> 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.

All the comments are handled.

The attached Patches contain all the suggested changes.

Thanks and Regards,
Shubham Khanna.