On Mon, Jan 17, 2022 12:35 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Sat, Jan 15, 2022 at 5:30 PM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > Attach the V65 patch set which addressed the above comments and Peter's > comments[1]. > > I also fixed some typos and removed some unused code. > > > > I have several minor comments for the v65-0001 patch:
Thanks for the comments ! > doc/src/sgml/ref/alter_subscription.sgml > > (1) > Suggest minor doc change: > > BEFORE: > + Previously-subscribed tables are not copied, even if the table's > row > + filter <literal>WHERE</literal> clause had been modified. > AFTER: > + Previously-subscribed tables are not copied, even if a table's row > + filter <literal>WHERE</literal> clause had been modified. > > > src/backend/catalog/pg_publication.c Changed. > (2) GetTopMostAncestorInPublication > Is there a reason why there is no "break" after finding a > topmost_relid? Why keep searching and potentially overwrite a > previously-found topmost_relid? If it's intentional, I think that a > comment should be added to explain it. The code was moved from get_rel_sync_entry, and was trying to get the last oid in the ancestor list which is published by the publication. Do you have some suggestions for the comment ? > > src/backend/commands/publicationcmds.c > > (3) Grammar > > BEFORE: > + * Returns true, if any of the columns used in row filter WHERE clause is not > AFTER: > + * Returns true, if any of the columns used in the row filter WHERE > clause are not Changed. > > (4) contain_invalid_rfcolumn_walker > Wouldn't this be better named "contains_invalid_rfcolumn_walker"? > (and references to the functions be updated accordingly) I am not sure about the name because many existing functions are named contain_xxx_xxx. (for example contain_mutable_functions) > > src/backend/executor/execReplication.c > > (5) Comment is difficult to read > Add commas to make the comment easier to read: > > BEFORE: > + * It is only safe to execute UPDATE/DELETE when all columns referenced in > + * the row filters from publications which the relation is in are valid - > AFTER: > + * It is only safe to execute UPDATE/DELETE when all columns, referenced in > + * the row filters from publications which the relation is in, are valid - > Changed. Best regards, Hou zj
