Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Pgoutput not capturing the generated columns |
Date | |
Msg-id | CAHut+PvST7be97xnxFjiHpz=WSr9MV567DLP4O-o7=Kb26pHTA@mail.gmail.com Whole thread Raw |
In response to | Re: Pgoutput not capturing the generated columns (Shubham Khanna <khannashubham1197@gmail.com>) |
Responses |
Re: Pgoutput not capturing the generated columns
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: