Re: row filtering for logical replication

On Wed, Dec 15, 2021 at 3:50 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Dec 13, 2021 at 8:49 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > PSA the v46* patch set.
> >
>
> 0001
>
...

> (2) In the 0001 patch comment, the term "publication filter" is used
> in one place, and in others "row filter" or "row-filter".
>

Fixed in v47 [1]

>
> src/backend/catalog/pg_publication.c
> (3) GetTransformedWhereClause() is missing a function comment.
>

Fixed in v47 [1]

> (4)
> The following comment seems incomplete:
>
> + /* Fix up collation information */
> + whereclause = GetTransformedWhereClause(pstate, pri, true);
>
>

Fixed in v47 [1]

> src/backend/parser/parse_relation.c
> (5)
> wording? consistent?
> Shouldn't it be "publication WHERE expression" for consistency?
>

In v47 [1]  this message is removed when the KIND is removed.

> + errmsg("publication row-filter WHERE invalid reference to table \"%s\"",
> + relation->relname),
>
>
> src/backend/replication/logical/tablesync.c
> (6)
>
> (i) Improve wording:
>
> BEFORE:
>  /*
>   * Get information about remote relation in similar fashion the RELATION
> - * message provides during replication.
> + * message provides during replication. This function also returns the relation
> + * qualifications to be used in COPY command.
>   */
>
> AFTER:
>  /*
> - * Get information about remote relation in similar fashion the RELATION
> - * message provides during replication.
> + * Get information about a remote relation, in a similar fashion to
> how the RELATION
> + * message provides information during replication. This function
> also returns the relation
> + * qualifications to be used in the COPY command.
>   */
>

Fixed in v47 [1]

> (ii) fetch_remote_table_info() doesn't currently account for ALL
> TABLES and ALL TABLES IN SCHEMA.
>
>

Fixed in v47 [1]

...

>
>
> 0002
>
> src/backend/catalog/pg_publication.c
> (1) rowfilter_walker()
> One of the errdetail messages doesn't begin with an uppercase letter:
>
> +   errdetail_msg = _("user-defined types are not allowed");
>
>

Fixed in v47 [1]

> src/backend/executor/execReplication.c
> (2) CheckCmdReplicaIdentity()
>
> Strictly speaking, the following:
>
> + if (invalid_rfcolnum)
>
> should be:
>
> + if (invalid_rfcolnum != InvalidAttrNumber)
>
>
Fixed in v47 [1]

> 0003
>
> src/backend/replication/logical/tablesync.c
> (1)
> Column name in comment should be "puballtables" not "puballtable":
>
> + * If any publication has puballtable true then all row-filtering is
>

Fixed in v47 [1]

> (2) pgoutput_row_filter_init()
>
> There should be a space before the final "*/" (so the asterisks align).
> Also, should say "... treated the same".
>
>   /*
> + * If the publication is FOR ALL TABLES then it is treated same as if this
> + * table has no filters (even if for some other publication it does).
> + */
>
>
Fixed in v47 [1]

------
[1] https://www.postgresql.org/message-id/CAHut%2BPtjsj_OVMWEdYp2Wq19%3DH5D4Vgta43FbFVDYr2LuS_djg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia