On Mon, Jan 17, 2022 12:35 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Sat, Jan 15, 2022 at 5:30 PM houzj.fnst@fujitsu.com
> <houzj.fnst@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