On Mon, Jun 24, 2024 at 8:21 AM Peter Smith <smithpb2...@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.
