Re: Pgoutput not capturing the generated columns

On Mon, Jul 1, 2024 at 8:38 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
>...
> > 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?
>

If you do manage to find a scenario for this then I think a test for
it would be good. But, I agree that the code seems OK because now I
see it is the same pattern as similar nearby code.

~~~

Thanks for the updated patch. Here are some review comments for patch v13-0001.

======
.../expected/generated_columns.out

nitpicks (see generated_columns.sql)

======
.../test_decoding/sql/generated_columns.sql

nitpick - use plural /column/columns/
nitpick - use consistent wording in the comments
nitpick - IMO it is better to INSERT different values for each of the tests

======
doc/src/sgml/protocol.sgml

nitpick - I noticed that none of the other boolean parameters on this
page mention about a default, so maybe here we should do the same and
omit that information.

~~~

1.
-     <para>
-      Next, the following message part appears for each column included in
-      the publication (except generated columns):
-     </para>
-

In a previous review [1 comment #11] I wrote that you can't just
remove this paragraph because AFAIK it is still meaningful. A minimal
change might be to just remove the "(except generated columns)" part.
Alternatively, you could give a more detailed explanation mentioning
the include_generated_columns protocol parameter.

I provided some updated text for this paragraph in my NITPICKS top-up
patch, Please have a look at that for ideas.

======
src/backend/commands/subscriptioncmds.c

It looks like pg_indent needs to be run on this file.

======
src/include/catalog/pg_subscription.h

nitpick - comment /publish/Publish/ for consistency

======
src/include/replication/walreceiver.h

nitpick - comment /publish/Publish/ for consistency

======
src/test/regress/expected/subscription.out

nitpicks - (see subscription.sql)

======
src/test/regress/sql/subscription.sql

nitpick - combine the invalid option combinations test with all the
others (no special comment needed)
nitpick - rename subscription as 'regress_testsub2' same as all its peers.

======
src/test/subscription/t/011_generated.pl

nitpick - add/remove blank lines

======
src/test/subscription/t/031_column_list.pl

nitpick - rewording for a comment. This issue was not strictly caused
by this patch, but since you are modifying the same comment we can fix
this in passing.

======
99.
Please also see the attached top-up patch for all those nitpicks
identified above.

======
[1] v11-0001 review
https://www.postgresql.org/message-id/CAHut%2BPv45gB4cV%2BSSs6730Kb8urQyqjdZ9PBVgmpwqCycr1Ybg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia