Re: row filtering for logical replication

On Tue, Nov 23, 2021 at 4:58 PM Ajin Cherian <itsajin(at)gmail(dot)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