v28-0003-POC-row-filter-walker-validation.patch

From c7e121cbc345a23fe4f8f41aea956b84d800750a Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 14 Sep 2021 12:27:03 +1000
Subject: [PATCH v28] POC row-filter walker validation

This patch implements a parse-tree "walker" to validate a row-filter expression.

Only very simple filer expression are permitted. Specifially:
- no user-defined operators.
- no functions.
- no parse nodes of any kind other than Var, OpExpr, Const, BoolExpr

This POC patch also disables (#if 0) all other row-filter errors which were thrown for
EXPR_KIND_PUBLICATION_WHERE in the 0001 patch.

Some regression tests are updated due to the modified validation error messages.
---
 src/backend/catalog/dependency.c          | 68 +++++++++++++++++++++++++++++++
 src/backend/catalog/pg_publication.c      | 14 +++++--
 src/backend/parser/parse_agg.c            |  5 ++-
 src/backend/parser/parse_expr.c           |  6 ++-
 src/backend/parser/parse_func.c           |  3 ++
 src/backend/parser/parse_oper.c           |  2 +
 src/include/catalog/dependency.h          |  2 +-
 src/test/regress/expected/publication.out | 17 +++++---
 src/test/regress/sql/publication.sql      |  2 +
 9 files changed, 107 insertions(+), 12 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index e81f093..405b3cd 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -132,6 +132,12 @@ typedef struct
 	int			subflags;		/* flags to pass down when recursing to obj */
 } ObjectAddressAndFlags;
 
+/* for rowfilter_walker */
+typedef struct
+{
+	char *relname;
+} rf_context;
+
 /* for find_expr_references_walker */
 typedef struct
 {
@@ -1554,6 +1560,68 @@ ReleaseDeletionLock(const ObjectAddress *object)
 }
 
 /*
+ * Walker checks that the row filter extression is legal. Allow only simple or
+ * or compound expressions like:
+ *
+ * "(Var Op Const)" or
+ * "(Var Op Const) Bool (Var Op Const)"
+ *
+ * Nothing more complicated is permitted. Specifically, no functions of any kind
+ * and no user-defined operators.
+ */
+static bool
+rowfilter_walker(Node *node, rf_context *context)
+{
+	char *forbidden = NULL;
+
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, Var) || IsA(node, Const) || IsA(node, BoolExpr))
+	{
+		/* OK */
+	}
+	else if (IsA(node, OpExpr))
+	{
+		/* OK, except user-defined operators are not allowed. */
+		if (((OpExpr *)node)->opno >= FirstNormalObjectId)
+			forbidden = _("user-defined operators are not allowed");
+	}
+	else if (IsA(node, FuncExpr))
+	{
+		forbidden = _("function calls are not allowed");
+	}
+	else
+	{
+		elog(DEBUG1, "row filter contained something unexpected: %s", nodeToString(node));
+		forbidden = _("too complex");
+	}
+
+	if (forbidden)
+		ereport(ERROR,
+				(errmsg("invalid publication WHERE expression for relation \"%s\"",
+						context->relname),
+				 errdetail("%s", forbidden),
+				 errhint("only simple expressions using columns and constants are allowed")
+				));
+
+	return expression_tree_walker(node, rowfilter_walker, (void *)context);
+}
+
+/*
+ * Walk the parse-tree of this publication row filter expression and throw an
+ * error if it encounters anything not permitted or unexpected.
+ */
+void
+rowfilter_validator(char *relname, Node *expr)
+{
+	rf_context context = {0};
+
+	context.relname = relname;
+	rowfilter_walker(expr, &context);
+}
+
+/*
  * Find all the columns referenced by the row-filter expression and return what
  * is found as a list of RfCol. This list is used for row-filter validation.
  */
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index ff2f28d..21ac56a 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -143,7 +143,7 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS)
  * Walk the parse-tree to decide if the row-filter is valid or not.
  */
 static void
-rowfilter_expr_checker(Publication *pub, Node *rfnode, Relation rel)
+rowfilter_expr_checker(Publication *pub, ParseState *pstate, Node *rfnode, Relation rel)
 {
 	Oid	relid = RelationGetRelid(rel);
 	char *relname = RelationGetRelationName(rel);
@@ -151,6 +151,14 @@ rowfilter_expr_checker(Publication *pub, Node *rfnode, Relation rel)
 	/*
 	 * Rule:
 	 *
+	 * Walk the parse-tree and reject anything more complicated than a very
+	 * simple expression.
+	 */
+	rowfilter_validator(relname, rfnode);
+
+	/*
+	 * Rule:
+	 *
 	 * If the publish operation contains "delete" then only columns that
 	 * are allowed by the REPLICA IDENTITY rules are permitted to be used in
 	 * the row-filter WHERE clause.
@@ -271,13 +279,13 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 		whereclause = transformWhereClause(pstate,
 										   copyObject(pri->whereClause),
 										   EXPR_KIND_PUBLICATION_WHERE,
-										   "PUBLICATION");
+										   "PUBLICATION WHERE");
 
 		/* Fix up collation information */
 		assign_expr_collations(pstate, whereclause);
 
 		/* Validate the row-filter. */
-		rowfilter_expr_checker(pub, whereclause, targetrel);
+		rowfilter_expr_checker(pub, pstate, whereclause, targetrel);
 	}
 
 	/* Form a tuple. */
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 193c87d..5e0c391 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -552,11 +552,12 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
 
 			break;
 		case EXPR_KIND_PUBLICATION_WHERE:
+#if 0
 			if (isAgg)
 				err = _("aggregate functions are not allowed in publication WHERE expressions");
 			else
 				err = _("grouping operations are not allowed in publication WHERE expressions");
-
+#endif
 			break;
 
 		case EXPR_KIND_CYCLE_MARK:
@@ -951,7 +952,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
 			errkind = true;
 			break;
 		case EXPR_KIND_PUBLICATION_WHERE:
+#if 0
 			err = _("window functions are not allowed in publication WHERE expressions");
+#endif
 			break;
 
 			/*
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 3210506..36b7e53 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -206,6 +206,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 
 		case T_FuncCall:
 			{
+#if 0
 				/*
 				 * Forbid functions in publication WHERE condition
 				 */
@@ -214,6 +215,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("functions are not allowed in publication WHERE expressions"),
 							 parser_errposition(pstate, exprLocation(expr))));
+#endif
 
 				result = transformFuncCall(pstate, (FuncCall *) expr);
 				break;
@@ -1782,7 +1784,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
 			err = _("cannot use subquery in column generation expression");
 			break;
 		case EXPR_KIND_PUBLICATION_WHERE:
+#if 0
 			err = _("cannot use subquery in publication WHERE expression");
+#endif
 			break;
 
 			/*
@@ -3105,7 +3109,7 @@ ParseExprKindName(ParseExprKind exprKind)
 		case EXPR_KIND_CYCLE_MARK:
 			return "CYCLE";
 		case EXPR_KIND_PUBLICATION_WHERE:
-			return "publication expression";
+			return "publication WHERE expression";
 
 			/*
 			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index e946f17..de9600f 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,7 +2656,10 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
 			errkind = true;
 			break;
 		case EXPR_KIND_PUBLICATION_WHERE:
+			pstate->p_hasTargetSRFs = true;
+#if 0
 			err = _("set-returning functions are not allowed in publication WHERE expressions");
+#endif
 			break;
 
 			/*
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index 29f8835..b3588df 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -718,12 +718,14 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree,
 											opform->oprright)),
 				 parser_errposition(pstate, location)));
 
+#if 0
 	/* Check it's not a custom operator for publication WHERE expressions */
 	if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE && opform->oid >= FirstNormalObjectId)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("user-defined operators are not allowed in publication WHERE expressions"),
 				 parser_errposition(pstate, location)));
+#endif
 
 	/* Do typecasting and build the expression tree */
 	if (ltree == NULL)
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 2c7310e..dd69aff 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -16,7 +16,6 @@
 
 #include "catalog/objectaddress.h"
 
-
 /*
  * Precise semantics of a dependency relationship are specified by the
  * DependencyType code (which is stored in a "char" field in pg_depend,
@@ -156,6 +155,7 @@ typedef struct RfCol {
 	int attnum;
 } RfCol;
 extern List *rowfilter_find_cols(Node *expr, Oid relId);
+extern void rowfilter_validator(char *relname, Node *expr);
 
 extern void recordDependencyOnExpr(const ObjectAddress *depender,
 								   Node *expr, List *rtable,
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index aa97e4d..237396e 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -209,16 +209,21 @@ Tables:
 
 -- fail - functions disallowed
 ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) < 6);
-ERROR:  functions are not allowed in publication WHERE expressions
-LINE 1: ...ICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) ...
-                                                             ^
+ERROR:  invalid publication WHERE expression for relation "testpub_rf_tbl4"
+DETAIL:  function calls are not allowed
+HINT:  only simple expressions using columns and constants are allowed
 -- fail - user-defined operators disallowed
 CREATE FUNCTION testpub_rf_func(integer, integer) RETURNS boolean AS $$ SELECT hashint4($1) > $2 $$ LANGUAGE SQL;
 CREATE OPERATOR =#> (PROCEDURE = testpub_rf_func, LEFTARG = integer, RIGHTARG = integer);
 CREATE PUBLICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27);
-ERROR:  user-defined operators are not allowed in publication WHERE expressions
-LINE 1: ...ICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27);
-                                                               ^
+ERROR:  invalid publication WHERE expression for relation "testpub_rf_tbl3"
+DETAIL:  user-defined operators are not allowed
+HINT:  only simple expressions using columns and constants are allowed
+-- fail - row-filter expression is not simple
+CREATE PUBLICATION testpub7 FOR TABLE testpub_rf_tbl1 WHERE (a IN (SELECT generate_series(1,5)));
+ERROR:  invalid publication WHERE expression for relation "testpub_rf_tbl1"
+DETAIL:  too complex
+HINT:  only simple expressions using columns and constants are allowed
 -- fail - WHERE not allowed in DROP
 ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27);
 ERROR:  syntax error at or near "WHERE"
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8552b36..faf7450 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -116,6 +116,8 @@ ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) < 6);
 CREATE FUNCTION testpub_rf_func(integer, integer) RETURNS boolean AS $$ SELECT hashint4($1) > $2 $$ LANGUAGE SQL;
 CREATE OPERATOR =#> (PROCEDURE = testpub_rf_func, LEFTARG = integer, RIGHTARG = integer);
 CREATE PUBLICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27);
+-- fail - row-filter expression is not simple
+CREATE PUBLICATION testpub7 FOR TABLE testpub_rf_tbl1 WHERE (a IN (SELECT generate_series(1,5)));
 -- fail - WHERE not allowed in DROP
 ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27);
 
-- 
1.8.3.1