Re: Pgoutput not capturing the generated columns

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: Shubham Khanna <khannashubham1197(at)gmail(dot)com>
Cc: Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, euler(at)eulerto(dot)com
Subject: Re: Pgoutput not capturing the generated columns
Date: 2024-06-24 02:51:26
Message-ID: CAHut+PvQ8CLq-JysTTeRj4u5SC9vTVcx3AgwTHcPUEOh-UnKcQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Kind Regards,
Peter Smith.
Fujitsu Australia.

Attachment Content-Type Size
PS_NITPICKS_20240624_v90001.txt text/plain 3.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiro.Ikeda 2024-06-24 02:52:10 RE: Improve EXPLAIN output for multicolumn B-Tree Index
Previous Message Masahiro.Ikeda 2024-06-24 02:38:32 RE: Improve EXPLAIN output for multicolumn B-Tree Index