Re: row filtering for logical replication

On Tue, Nov 23, 2021 at 4:58 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Attaching a new patchset v41 which includes changes by both Peter and myself.
>
> Patches v40-0005 and v40-0006 have been merged to create patch
> v41-0005 which reduces the patches to 6 again.

Few comments:
1) I'm not sure if we will be able to throw a better error message in
this case "ERROR:  missing FROM-clause entry for table "t4"", if
possible you could change it.

+       if (pri->whereClause != NULL)
+       {
+               /* Set up a pstate to parse with */
+               pstate = make_parsestate(NULL);
+               pstate->p_sourcetext = nodeToString(pri->whereClause);
+
+               nsitem = addRangeTableEntryForRelation(pstate, targetrel,
+
                    AccessShareLock,
+
                    NULL, false, false);
+               addNSItemToQuery(pstate, nsitem, false, true, true);
+
+               whereclause = transformWhereClause(pstate,
+
            copyObject(pri->whereClause),
+
            EXPR_KIND_PUBLICATION_WHERE,
+
            "PUBLICATION");
+
+               /* Fix up collation information */
+               assign_expr_collations(pstate, whereclause);
+       }

alter publication pub1 add table t5 where ( t4.c1 = 10);
ERROR:  missing FROM-clause entry for table "t4"
LINE 1: alter publication pub1 add table t5 where ( t4.c1 = 10);
                                                    ^
pstate->p_expr_kind is stored as EXPR_KIND_PUBLICATION_WHERE, we could
differentiate using expr_kind.

2) Should '"delete" or "delete"' be '"delete" or "update"'
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -340,7 +340,7 @@ rowfilter_walker(Node *node, rf_context *context)
  * 1. Only certain simple node types are permitted in the expression. See
  * function rowfilter_walker for details.
  *
- * 2. If the publish operation contains "delete" then only columns that
+ * 2. If the publish operation contains "delete" or "delete" then
only columns that
  * are allowed by the REPLICA IDENTITY rules are permitted to be used in the
  * row-filter WHERE clause.
  */
@@ -352,12 +352,10 @@ rowfilter_expr_checker(Publication *pub,
Relation rel, Node *rfnode)
        context.rel = rel;

        /*
-        * For "delete", check that filter cols are also valid replica identity
+        * For "delete" or "update", check that filter cols are also
valid replica identity
         * cols.

3) Should we include row filter condition in pg_publication_tables
view like in describe publication(\dRp+) , since the prqual is not
easily readable in pg_publication_rel table:
select * from pg_publication_tables ;
 pubname | schemaname | tablename
---------+------------+-----------
 pub1    | public     | t1
(1 row)

 select * from pg_publication_rel ;
  oid  | prpubid | prrelid |
      prqual

-------+---------+---------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 16389 |   16388 |   16384 | {OPEXPR :opno 518 :opfuncid 144
:opresulttype 16 :opretset false :opcollid 0 :inputcollid 0 :args
({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0
:varlevelsup 0 :va
rnosyn 1 :varattnosyn 1 :location 45} {CONST :consttype 23
:consttypmod -1 :constcollid 0 :constlen 4 :constbyval true
:constisnull false :location 51 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]})
:location 48}
(1 row)

4) This should be included in typedefs.list, also we could add some
comments for this structure
+typedef struct {
+       Relation        rel;
+       Bitmapset  *bms_replident;
+}
+rf_context;

5) Few includes are not required. #include "miscadmin.h" not required
in pg_publication.c, #include "executor/executor.h" not required in
proto.c, #include "access/xact.h", #include "executor/executor.h" and
#include "replication/logicalrelation.h" not required in pgoutput.c

6) typo "filte" should be "filter":
+/*
+ * The row filte walker checks that the row filter expression is legal.
+ *
+ * Rules: Node-type validation
+ * ---------------------------
+ * Allow only simple or compound expressions like:
+ * - "(Var Op Const)" or
+ * - "(Var Op Const) Bool (Var Op Const)"

Regards,
Vignesh