v36-0005-PS-Row-filter-validation-walker.patch

From 9b5b53181a278d79eef701884a586906b5f4dd65 Mon Sep 17 00:00:00 2001
From: Ajin Cherian <ajinc@fast.au.fujitsu.com>
Date: Wed, 3 Nov 2021 08:11:57 -0400
Subject: [PATCH v36 5/6] PS - Row filter validation walker.

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

Only very simple filter expressions are permitted. Specifially:
- no user-defined operators.
- no user-defined functions.
- no system functions (unless they are IMMUTABLE). See design decision at [1].
- no parse nodes of any kind other than Var, OpExpr, Const, BoolExpr, FuncExpr
[1] https://www.postgresql.org/message-id/CAA4eK1%2BXoD49bz5-2TtiD0ugq4PHSRX2D1sLPR_X4LNtdMc4OQ%40mail.gmail.com

This 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 modified validation messages and rules.
---
 src/backend/catalog/dependency.c          | 93 +++++++++++++++++++++++++++++++
 src/backend/catalog/pg_publication.c      | 20 ++++---
 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 | 25 ++++++---
 src/test/regress/sql/publication.sql      | 11 +++-
 9 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index de7ff90..fbf8d78 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -133,6 +133,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
 {
@@ -1560,6 +1566,93 @@ 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)"
+ *
+ * User-defined operators are not allowed.
+ * User-defined functions are not allowed.
+ * System functions that are not IMMUTABLE are not allowed.
+ */
+static bool
+rowfilter_walker(Node *node, rf_context *context)
+{
+	char *forbidden = NULL;
+	bool too_complex = false;
+
+	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))
+	{
+		Oid funcid = ((FuncExpr *)node)->funcid;
+		char *funcname = get_func_name(funcid);
+
+		/*
+		 * User-defined functions are not allowed.
+		 * System-functions that are not IMMUTABLE are not allowed.
+		 */
+		if (funcid >= FirstNormalObjectId)
+		{
+			forbidden = psprintf("user-defined functions are not allowed: %s",
+								 funcname);
+		}
+		else
+		{
+			if (func_volatile(funcid) != PROVOLATILE_IMMUTABLE)
+				forbidden = psprintf("system functions that are not IMMUTABLE are not allowed: %s",
+									 funcname);
+		}
+	}
+	else
+	{
+		elog(DEBUG1, "the row filter contained something unexpected: %s", nodeToString(node));
+		too_complex = true;
+	}
+
+	if (too_complex)
+		ereport(ERROR,
+				(errmsg("invalid publication WHERE expression for relation \"%s\"",
+						context->relname),
+				 errhint("only simple expressions using columns, constants and immutable system functions are allowed")
+				));
+
+	if (forbidden)
+		ereport(ERROR,
+				(errmsg("invalid publication WHERE expression for relation \"%s\"",
+						context->relname),
+				 errdetail("%s", forbidden)
+				));
+
+	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 them
  * as a list of attribute numbers. 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 e04e069..f56e01f 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -216,20 +216,26 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS)
 /*
  * Decide if the row-filter is valid according to the following rules:
  *
- * Rule 1. If the publish operation contains "delete" then only columns that
+ * Rule 1. Walk the parse-tree and reject anything other than very simple
+ * expressions. (See rowfilter_validator for details what is permitted).
+ *
+ * Rule 2. 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.
- *
- * Rule 2. TODO
  */
 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);
 
 	/*
-	 * Rule 1: For "delete", check that filter cols are also valid replica
+	 * Rule 1. Walk the parse-tree and reject anything unexpected.
+	 */
+	rowfilter_validator(relname, rfnode);
+
+	/*
+	 * Rule 2: For "delete", check that filter cols are also valid replica
 	 * identity cols.
 	 *
 	 * TODO - check later for publish "update" case.
@@ -381,13 +387,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 3d43839..3519e62 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -201,6 +201,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 
 		case T_FuncCall:
 			{
+#if 0
 				/*
 				 * Forbid functions in publication WHERE condition
 				 */
@@ -209,6 +210,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;
@@ -1777,7 +1779,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;
 
 			/*
@@ -3100,7 +3104,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 2427321..e118256 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,
@@ -153,6 +152,7 @@ extern void performMultipleDeletions(const ObjectAddresses *objects,
 									 DropBehavior behavior, int flags);
 
 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 72f8dff..79b1955 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -308,17 +308,27 @@ RESET client_min_messages;
 DROP PUBLICATION testpub_syntax2;
 ERROR:  publication "testpub_syntax2" does not exist
 -- 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) ...
-                                                             ^
+-- fail - user-defined functions are not allowed
+CREATE FUNCTION testpub_rf_func99() RETURNS integer AS $$ BEGIN RETURN 99; END; $$ LANGUAGE plpgsql;
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < testpub_rf_func99());
+ERROR:  invalid publication WHERE expression for relation "testpub_rf_tbl1"
+DETAIL:  user-defined functions are not allowed: testpub_rf_func99
+-- fail - system functions that are not IMMUTABLE are not allowed; random() is a "volatile" function.
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < random());
+ERROR:  invalid publication WHERE expression for relation "testpub_rf_tbl1"
+DETAIL:  system functions that are not IMMUTABLE are not allowed: random
+-- ok - system functions that are IMMUTABLE are allowed; int8inc() is an "immutable" function.
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < int8inc(999));
 -- 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
+-- 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"
+HINT:  only simple expressions using columns, constants and immutable system functions are allowed
 -- fail - WHERE not allowed in DROP
 ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27);
 ERROR:  invalid use of WHERE row-filter in ALTER PUBLICATION ... DROP TABLE
@@ -333,6 +343,7 @@ DROP SCHEMA testpub_rf_myschema;
 DROP PUBLICATION testpub5;
 DROP OPERATOR =#>(integer, integer);
 DROP FUNCTION testpub_rf_func(integer, integer);
+DROP FUNCTION testpub_rf_func99();
 -- ======================================================
 -- More row filter tests for validating column references
 CREATE TABLE rf_tbl_abcd_nopk(a int, b int, c int, d int);
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 2f291eb..6faa1de 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -165,11 +165,19 @@ RESET client_min_messages;
 \dRp+ testpub_syntax2
 DROP PUBLICATION testpub_syntax2;
 -- fail - functions disallowed
-ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl4 WHERE (length(g) < 6);
+-- fail - user-defined functions are not allowed
+CREATE FUNCTION testpub_rf_func99() RETURNS integer AS $$ BEGIN RETURN 99; END; $$ LANGUAGE plpgsql;
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < testpub_rf_func99());
+-- fail - system functions that are not IMMUTABLE are not allowed; random() is a "volatile" function.
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < random());
+-- ok - system functions that are IMMUTABLE are allowed; int8inc() is an "immutable" function.
+ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a < int8inc(999));
 -- 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);
+-- 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);
 
@@ -182,6 +190,7 @@ DROP SCHEMA testpub_rf_myschema;
 DROP PUBLICATION testpub5;
 DROP OPERATOR =#>(integer, integer);
 DROP FUNCTION testpub_rf_func(integer, integer);
+DROP FUNCTION testpub_rf_func99();
 
 -- ======================================================
 -- More row filter tests for validating column references
-- 
1.8.3.1