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.