Re: row filtering for logical replication - Mailing list pgsql-hackers

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.

~~

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.

~~

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.

~~

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.

~~

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"

~~

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"

~~

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"

~~

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.

~~

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

~~

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" ?

~~

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.

~~

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.

~~

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"

------
Kind Regards,
Peter Smith.
Fujitsu Australia.