Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers

On Mon, Jun 24, 2024 at 8:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi, here are some patch v9-0001 comments.
>
> I saw Kuroda-san has already posted comments for this patch so there
> may be some duplication here.
>
> ======
> GENERAL
>
> 1.
> The later patches 0002 etc are checking to support only STORED
> gencols. But, doesn't that restriction belong in this patch 0001 so
> VIRTUAL columns are not decoded etc in the first place... (??)
>
> ~~~
>
> 2.
> The "Generated Columns" docs mentioned in my previous review comment
> [2] should be modified by this 0001 patch.
>
> ~~~
>
> 3.
> I think the "Message Format" page mentioned in my previous review
> comment [3] should be modified by this 0001 patch.
>
> ======
> Commit message
>
> 4.
> The patch name is still broken as previously mentioned [1, #1]
>
> ======
> doc/src/sgml/protocol.sgml
>
> 5.
> Should this docs be referring to STORED generated columns, instead of
> just generated columns?
>
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 6.
> Should this be docs referring to STORED generated columns, instead of
> just generated columns?
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> getSubscriptions:
> NITPICK - tabs
> NITPICK - patch removed a blank line it should not be touching
> NITPICK = patch altered indents it should not be touching
> NITPICK - a missing blank line that was previously present
>
> 7.
> + else
> + appendPQExpBufferStr(query,
> + " false AS subincludegencols,\n");
>
> There is an unwanted comma here.
>
> ~
>
> dumpSubscription
> NITPICK - patch altered indents it should not be touching
>
> ======
> src/bin/pg_dump/pg_dump.h
>
> NITPICK - unnecessary blank line
>
> ======
> src/bin/psql/describe.c
>
> describeSubscriptions
> NITPICK - bad indentation
>
> 8.
> In my previous review [1, #4b] I suggested this new column should be
> in a different order (e.g. adjacent to the other ones ahead of
> 'Conninfo'), but this is not yet addressed.
>
> ======
> src/test/subscription/t/011_generated.pl
>
> NITPICK - missing space in comment
> NITPICK - misleading "because" wording in the comment
>
> ======
>
> 99.
> See also my attached nitpicks diff, for cosmetic issues. Please apply
> whatever you agree with.
>
> ======
> [1] My v8-0001 review -
> https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com
> [3] https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i2g%40mail.gmail.com

All the comments are handled.

I have attached the updated patch v11 here in [1]. See [1] for the
changes added.

[1] https://www.postgresql.org/message-id/CAHv8RjJpS_XDkR6OrsmMZtCBZNxeYoCdENhC0%3Dbe0rLmNvhiQw%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.