Re: row filtering for logical replication

On Wed, Jan 5, 2022 at 4:34 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> I have reviewed again the source code for v58-0001.
>
> Below are my review comments.
>
> Actually, I intend to fix most of these myself for v59*, so this post
> is just for records.
>
> v58-0001 Review Comments
> ========================
>
> 1. doc/src/sgml/ref/alter_publication.sgml - reword for consistency
>
> +      name to explicitly indicate that descendant tables are included. If the
> +      optional <literal>WHERE</literal> clause is specified, rows that do not
> +      satisfy the <replaceable class="parameter">expression</replaceable> will
> +      not be published. Note that parentheses are required around the
>
> For consistency, it would be better to reword this sentence about the
> expression to be more similar to the one in CREATE PUBLICATION, which
> now says:
>
> +      If the optional <literal>WHERE</literal> clause is specified, rows for
> +      which the <replaceable class="parameter">expression</replaceable> returns
> +      false or null will not be published. Note that parentheses are required
> +      around the expression. It has no effect on <literal>TRUNCATE</literal>
> +      commands.
>

Updated in v59* [1]

> ~~
>
> 2. doc/src/sgml/ref/create_subscription.sgml - reword for consistency
>
> @@ -319,6 +324,25 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
>     the parameter <literal>create_slot = false</literal>.  This is an
>     implementation restriction that might be lifted in a future release.
>    </para>
> +
> +  <para>
> +   If any table in the publication has a <literal>WHERE</literal> clause, rows
> +   that do not satisfy the <replaceable
> class="parameter">expression</replaceable>
> +   will not be published. If the subscription has several publications in which
>
> For consistency, it would be better to reword this sentence about the
> expression to be more similar to the one in CREATE PUBLICATION, which
> now says:
>
> +      If the optional <literal>WHERE</literal> clause is specified, rows for
> +      which the <replaceable class="parameter">expression</replaceable> returns
> +      false or null will not be published. Note that parentheses are required
> +      around the expression. It has no effect on <literal>TRUNCATE</literal>
> +      commands.
>

Updated in v59* [1]

> ~~
>
> 3. src/backend/catalog/pg_publication.c - whitespace
>
> +rowfilter_walker(Node *node, Relation relation)
> +{
> + char    *errdetail_msg = NULL;
> +
> + if (node == NULL)
> + return false;
> +
> +
> + if (IsRowFilterSimpleExpr(node))
>
> Remove the extra blank line.
>

Fixed in v59* [1]

> ~~
>
> 4. src/backend/executor/execReplication.c - move code
>
> + bad_rfcolnum = GetRelationPublicationInfo(rel, true);
> +
> + /*
> + * 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,
> + * which means all referenced columns are part of REPLICA IDENTITY, or the
> + * table do not publish UPDATES or DELETES.
> + */
> + if (AttributeNumberIsValid(bad_rfcolnum))
>
> I felt that the bad_rfcolnum assignment belongs below the large
> comment explaining this logic.
>

Fixed in v59* [1]

> ~~
>
> 5. src/backend/executor/execReplication.c - fix typo
>
> + /*
> + * 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,
> + * which means all referenced columns are part of REPLICA IDENTITY, or the
> + * table do not publish UPDATES or DELETES.
> + */
>
> Typo: "table do not publish" -> "table does not publish"
>

Fixed in v59* [1]

> ~~
>
> 6. src/backend/replication/pgoutput/pgoutput.c - fix typo
>
> + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> + /* Gather the rfnodes per pubaction of this publiaction. */
> + if (pub->pubactions.pubinsert)
>
> Typo: "publiaction" --> "publication"
>

Fixed in v59* [1]

> ~~
>
> 7. src/backend/utils/cache/relcache.c - fix comment case
>
> @@ -267,6 +271,19 @@ typedef struct opclasscacheent
>
>  static HTAB *OpClassCache = NULL;
>
> +/*
> + * Information used to validate the columns in the row filter expression. see
> + * rowfilter_column_walker for details.
> + */
>
> Typo: "see" --> "See"
>

Fixed in v59* [1]

> ~~
>
> 8. src/backend/utils/cache/relcache.c - "row-filter"
>
> For consistency with all other naming change all instances of
> "row-filter" to "row filter" in this file.
>

Fixed in v59* [1]

> ~~
>
> 9. src/backend/utils/cache/relcache.c - fix typo
>

Fixed in v59* [1]

> ~~
>
> 10. src/backend/utils/cache/relcache.c - comment confused wording?
>
> Function GetRelationPublicationInfo:
>
> + /*
> + * For a partition, if pubviaroot is true, check if any of the
> + * ancestors are published. If so, note down the topmost ancestor
> + * that is published via this publication, the row filter
> + * expression on which will be used to filter the partition's
> + * changes. We could have got the topmost ancestor when collecting
> + * the publication oids, but that will make the code more
> + * complicated.
> + */
>
> Typo: Probably "on which' --> "of which" ?
>

Fixed in v59* [1]

> ~~
>
> 11. src/backend/utils/cache/relcache.c - GetRelationPublicationActions
>
> Something seemed slightly fishy with the code doing the memcpy,
> because IIUC is possible for the GetRelationPublicationInfo function
> to return without setting the relation->rd_pubactions. Is it just
> missing an Assert or maybe a comment to say such a scenario is not
> possible in this case because the is_publishable_relation was already
> tested?
>
> Currently, it just seems a little bit too sneaky.
>

TODO

> ~~
>
> 12. src/include/parser/parse_node.h - This change is unrelated to row-filtering.
>
> @@ -79,7 +79,7 @@ typedef enum ParseExprKind
>   EXPR_KIND_CALL_ARGUMENT, /* procedure argument in CALL */
>   EXPR_KIND_COPY_WHERE, /* WHERE condition in COPY FROM */
>   EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
> - EXPR_KIND_CYCLE_MARK, /* cycle mark value */
> + EXPR_KIND_CYCLE_MARK /* cycle mark value */
>  } ParseExprKind;
>
> This change is unrelated to Row-Filtering so ought to be removed from
> this patch. Soon I will post a separate thread to fix this
> independently on HEAD.
>

Fixed in v59* [1].

I started a separate thread for this problem.
See https://www.postgresql.org/message-id/flat/CAHut%2BPsqr93nng7diTXxtUD636u7ytA%3DMq2duRphs0CBzpfDTA%40mail.gmail.com

> ~~
>
> 13. src/include/utils/rel.h - comment typos
>
> @@ -164,6 +164,13 @@ typedef struct RelationData
>   PublicationActions *rd_pubactions; /* publication actions */
>
>   /*
> + * true if the columns referenced in row filters from all the publications
> + * the relation is in are part of replica identity, or the publication
> + * actions do not include UPDATE and DELETE.
> + */
>
> Some minor rewording of the comment:
>
> "true" --> "True".
> "part of replica identity" --> "part of the replica identity"
> "UPDATE and DELETE" --> "UPDATE or DELETE"
>

Fixed in v59* [1]

------
[1] https://www.postgresql.org/message-id/CAHut%2BPsiw9fbOUTpCMWirut1ZD5hbWk8_U9tZya4mG-YK%2Bfq8g%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia