RE: row filtering for logical replication

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