On Tue, Jul 2, 2024 at 9:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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
All the comments are handled.
The attached Patches contain all the suggested changes. Here, v15-0001
is modified to fix the comments, v15-0002 is not modified and v15-0003
is modified according to the changes in v15-0001 patch.
Thanks Shlok Kyal for modifying the v15-0003 Patch.
Thanks and Regards,
Shubham Khanna.
