v57-0004-refactor-row-filter-transformation.patch

From eca2ad55c2365269e3430e8f012a4acf3ffa42f4 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Thu, 30 Dec 2021 21:13:39 +0800
Subject: [PATCH v57] refactor row filter transformation

Based on the division of labor, publicationcmds.c is more natural to handle the
expression transformation while pg_publication.c should only deal with the
pg_publication tuples.

Before this patch, we could transform the row filter expression both in
pg_publication.c and publicationcmd.c. This patch move the all the node
transformation to publicationcmds.c in AlterPublicationTables() and
CreatePublication() to match the division of labor which also avoid extra
transformation.

Besides, pass the queryString to the AlterPublicationTables(), so that it can
be used when transforming the expression to report correct error position.
---
 src/backend/catalog/pg_publication.c      | 196 +-------------------------
 src/backend/commands/publicationcmds.c    | 219 +++++++++++++++++++++++++++---
 src/include/catalog/pg_publication.h      |   3 -
 src/test/regress/expected/publication.out |   4 +-
 4 files changed, 205 insertions(+), 217 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 3818326..ec57003 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -29,7 +29,6 @@
 #include "catalog/objectaddress.h"
 #include "catalog/pg_inherits.h"
 #include "catalog/pg_namespace.h"
-#include "catalog/pg_proc.h"
 #include "catalog/pg_publication.h"
 #include "catalog/pg_publication_namespace.h"
 #include "catalog/pg_publication_rel.h"
@@ -37,10 +36,6 @@
 #include "commands/publicationcmds.h"
 #include "funcapi.h"
 #include "miscadmin.h"
-#include "nodes/nodeFuncs.h"
-#include "parser/parse_clause.h"
-#include "parser/parse_collate.h"
-#include "parser/parse_relation.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/catcache.h"
@@ -114,136 +109,6 @@ check_publication_add_schema(Oid schemaid)
 }
 
 /*
- * Is this a simple Node permitted within a row filter expression?
- */
-static bool
-IsRowFilterSimpleExpr(Node *node)
-{
-	switch (nodeTag(node))
-	{
-		case T_ArrayExpr:
-		case T_BooleanTest:
-		case T_BoolExpr:
-		case T_CaseExpr:
-		case T_CaseTestExpr:
-		case T_CoalesceExpr:
-		case T_Const:
-		case T_List:
-		case T_MinMaxExpr:
-		case T_NullIfExpr:
-		case T_NullTest:
-		case T_ScalarArrayOpExpr:
-		case T_XmlExpr:
-			return true;
-		default:
-			return false;
-	}
-}
-
-/*
- * The row filter walker checks if the row filter expression is a "simple
- * expression".
- *
- * It allows only simple or compound expressions such as:
- * - (Var Op Const)
- * - (Var Op Var)
- * - (Var Op Const) Bool (Var Op Const)
- * - etc
- * (where Var is a column of the table this filter belongs to)
- *
- * The simple expression contains the following restrictions:
- * - User-defined operators are not allowed;
- * - User-defined functions are not allowed;
- * - User-defined types are not allowed;
- * - Non-immutable built-in functions are not allowed;
- * - System columns are not allowed.
- *
- * NOTES
- *
- * We don't allow user-defined functions/operators/types because
- * (a) if a user drops a user-defined object used in a row filter expression or
- * if there is any other error while using it, the logical decoding
- * infrastructure won't be able to recover from such an error even if the
- * object is recreated again because a historic snapshot is used to evaluate
- * the row filter;
- * (b) a user-defined function can be used to access tables which could have
- * unpleasant results because a historic snapshot is used. That's why only
- * immutable built-in functions are allowed in row filter expressions.
- */
-static bool
-rowfilter_walker(Node *node, Relation relation)
-{
-	char	   *errdetail_msg = NULL;
-
-	if (node == NULL)
-		return false;
-
-
-	if (IsRowFilterSimpleExpr(node))
-	{
-		/* OK, node is part of simple expressions */
-	}
-	else if (IsA(node, Var))
-	{
-		Var		   *var = (Var *) node;
-
-		/* User-defined types are not allowed. */
-		if (var->vartype >= FirstNormalObjectId)
-			errdetail_msg = _("User-defined types are not allowed.");
-
-		/* System columns are not allowed. */
-		else if (var->varattno < InvalidAttrNumber)
-		{
-			Oid			relid = RelationGetRelid(relation);
-			const char *colname = get_attname(relid, var->varattno, false);
-
-			errdetail_msg = psprintf(_("Cannot use system column (%s)."), colname);
-		}
-	}
-	else if (IsA(node, OpExpr))
-	{
-		/* OK, except user-defined operators are not allowed. */
-		if (((OpExpr *) node)->opno >= FirstNormalObjectId)
-			errdetail_msg = _("User-defined operators are not allowed.");
-	}
-	else if (IsA(node, FuncExpr))
-	{
-		Oid			funcid = ((FuncExpr *) node)->funcid;
-		const char *funcname = get_func_name(funcid);
-
-		/*
-		 * User-defined functions are not allowed. System-functions that are
-		 * not IMMUTABLE are not allowed.
-		 */
-		if (funcid >= FirstNormalObjectId)
-			errdetail_msg = psprintf(_("User-defined functions are not allowed (%s)."),
-									 funcname);
-		else if (func_volatile(funcid) != PROVOLATILE_IMMUTABLE)
-			errdetail_msg = psprintf(_("Non-immutable built-in functions are not allowed (%s)."),
-									 funcname);
-	}
-	else
-	{
-		elog(DEBUG3, "row filter contains an unexpected expression component: %s", nodeToString(node));
-
-		ereport(ERROR,
-				(errmsg("invalid publication WHERE expression for relation \"%s\"",
-						RelationGetRelationName(relation)),
-				 errdetail("Expressions only allow columns, constants, built-in operators, built-in data types and non-immutable built-in functions.")
-				 ));
-	}
-
-	if (errdetail_msg)
-		ereport(ERROR,
-				(errmsg("invalid publication WHERE expression for relation \"%s\"",
-						RelationGetRelationName(relation)),
-				 errdetail("%s", errdetail_msg)
-				 ));
-
-	return expression_tree_walker(node, rowfilter_walker, (void *) relation);
-}
-
-/*
  * Returns if relation represented by oid and Form_pg_class entry
  * is publishable.
  *
@@ -411,36 +276,6 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
 }
 
 /*
- * Transform a publication WHERE clause, ensuring it is coerced to boolean and
- * necessary collation information is added if required, and add a new
- * nsitem/RTE for the associated relation to the ParseState's namespace list.
- */
-Node *
-GetTransformedWhereClause(ParseState *pstate, PublicationRelInfo *pri,
-						  bool fixup_collation)
-{
-	ParseNamespaceItem *nsitem;
-	Node	   *whereclause = NULL;
-
-	pstate->p_sourcetext = nodeToString(pri->whereClause);
-
-	nsitem = addRangeTableEntryForRelation(pstate, pri->relation,
-										   AccessShareLock, NULL, false, false);
-
-	addNSItemToQuery(pstate, nsitem, false, true, true);
-
-	whereclause = transformWhereClause(pstate, copyObject(pri->whereClause),
-									   EXPR_KIND_WHERE,
-									   "PUBLICATION WHERE");
-
-	/* Fix up collation information */
-	if (fixup_collation)
-		assign_expr_collations(pstate, whereclause);
-
-	return whereclause;
-}
-
-/*
  * Check if any of the ancestors are published in the publication. If so,
  * return the relid of the topmost ancestor that is published via this
  * publication, otherwise InvalidOid.
@@ -485,8 +320,6 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 	Publication *pub = GetPublication(pubid);
 	ObjectAddress myself,
 				referenced;
-	ParseState *pstate;
-	Node	   *whereclause = NULL;
 	List	   *relids = NIL;
 
 	rel = table_open(PublicationRelRelationId, RowExclusiveLock);
@@ -526,25 +359,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 
 	/* Add qualifications, if available */
 	if (pri->whereClause != NULL)
-	{
-		/* Set up a ParseState to parse with */
-		pstate = make_parsestate(NULL);
-
-		/*
-		 * Get the transformed WHERE clause, of boolean type, with necessary
-		 * collation information.
-		 */
-		whereclause = GetTransformedWhereClause(pstate, pri, true);
-
-		/*
-		 * Walk the parse-tree of this publication row filter expression and
-		 * throw an error if anything not permitted or unexpected is
-		 * encountered.
-		 */
-		rowfilter_walker(whereclause, targetrel);
-
-		values[Anum_pg_publication_rel_prqual - 1] = CStringGetTextDatum(nodeToString(whereclause));
-	}
+		values[Anum_pg_publication_rel_prqual - 1] = CStringGetTextDatum(nodeToString(pri->whereClause));
 	else
 		nulls[Anum_pg_publication_rel_prqual - 1] = true;
 
@@ -565,11 +380,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 	recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
 
 	/* Add dependency on the objects mentioned in the qualifications */
-	if (whereclause)
-	{
-		recordDependencyOnExpr(&myself, whereclause, pstate->p_rtable, DEPENDENCY_NORMAL);
-		free_parsestate(pstate);
-	}
+	if (pri->whereClause)
+		recordDependencyOnSingleRelExpr(&myself, pri->whereClause, relid,
+										DEPENDENCY_NORMAL, DEPENDENCY_NORMAL,
+										false);
 
 	/* Close the table. */
 	table_close(rel, RowExclusiveLock);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 61b0ff2..424319a 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -26,6 +26,7 @@
 #include "catalog/partition.h"
 #include "catalog/pg_inherits.h"
 #include "catalog/pg_namespace.h"
+#include "catalog/pg_proc.h"
 #include "catalog/pg_publication.h"
 #include "catalog/pg_publication_namespace.h"
 #include "catalog/pg_publication_rel.h"
@@ -36,6 +37,10 @@
 #include "commands/publicationcmds.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
+#include "parser/parse_clause.h"
+#include "parser/parse_collate.h"
+#include "parser/parse_relation.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/array.h"
@@ -235,6 +240,188 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
 }
 
 /*
+ * Is this a simple Node permitted within a row filter expression?
+ */
+static bool
+IsRowFilterSimpleExpr(Node *node)
+{
+	switch (nodeTag(node))
+	{
+		case T_ArrayExpr:
+		case T_BooleanTest:
+		case T_BoolExpr:
+		case T_CaseExpr:
+		case T_CaseTestExpr:
+		case T_CoalesceExpr:
+		case T_Const:
+		case T_List:
+		case T_MinMaxExpr:
+		case T_NullIfExpr:
+		case T_NullTest:
+		case T_ScalarArrayOpExpr:
+		case T_XmlExpr:
+			return true;
+		default:
+			return false;
+	}
+}
+
+/*
+ * The row filter walker checks if the row filter expression is a "simple
+ * expression".
+ *
+ * It allows only simple or compound expressions such as:
+ * - (Var Op Const)
+ * - (Var Op Var)
+ * - (Var Op Const) Bool (Var Op Const)
+ * - etc
+ * (where Var is a column of the table this filter belongs to)
+ *
+ * The simple expression contains the following restrictions:
+ * - User-defined operators are not allowed;
+ * - User-defined functions are not allowed;
+ * - User-defined types are not allowed;
+ * - Non-immutable built-in functions are not allowed;
+ * - System columns are not allowed.
+ *
+ * NOTES
+ *
+ * We don't allow user-defined functions/operators/types because
+ * (a) if a user drops a user-defined object used in a row filter expression or
+ * if there is any other error while using it, the logical decoding
+ * infrastructure won't be able to recover from such an error even if the
+ * object is recreated again because a historic snapshot is used to evaluate
+ * the row filter;
+ * (b) a user-defined function can be used to access tables which could have
+ * unpleasant results because a historic snapshot is used. That's why only
+ * immutable built-in functions are allowed in row filter expressions.
+ */
+static bool
+rowfilter_walker(Node *node, Relation relation)
+{
+	char	   *errdetail_msg = NULL;
+
+	if (node == NULL)
+		return false;
+
+
+	if (IsRowFilterSimpleExpr(node))
+	{
+		/* OK, node is part of simple expressions */
+	}
+	else if (IsA(node, Var))
+	{
+		Var		   *var = (Var *) node;
+
+		/* User-defined types are not allowed. */
+		if (var->vartype >= FirstNormalObjectId)
+			errdetail_msg = _("User-defined types are not allowed.");
+
+		/* System columns are not allowed. */
+		else if (var->varattno < InvalidAttrNumber)
+		{
+			Oid			relid = RelationGetRelid(relation);
+			const char *colname = get_attname(relid, var->varattno, false);
+
+			errdetail_msg = psprintf(_("Cannot use system column (%s)."), colname);
+		}
+	}
+	else if (IsA(node, OpExpr))
+	{
+		/* OK, except user-defined operators are not allowed. */
+		if (((OpExpr *) node)->opno >= FirstNormalObjectId)
+			errdetail_msg = _("User-defined operators are not allowed.");
+	}
+	else if (IsA(node, FuncExpr))
+	{
+		Oid			funcid = ((FuncExpr *) node)->funcid;
+		const char *funcname = get_func_name(funcid);
+
+		/*
+		 * User-defined functions are not allowed. System-functions that are
+		 * not IMMUTABLE are not allowed.
+		 */
+		if (funcid >= FirstNormalObjectId)
+			errdetail_msg = psprintf(_("User-defined functions are not allowed (%s)."),
+									 funcname);
+		else if (func_volatile(funcid) != PROVOLATILE_IMMUTABLE)
+			errdetail_msg = psprintf(_("Non-immutable built-in functions are not allowed (%s)."),
+									 funcname);
+	}
+	else
+	{
+		elog(DEBUG3, "row filter contains an unexpected expression component: %s", nodeToString(node));
+
+		ereport(ERROR,
+				(errmsg("invalid publication WHERE expression for relation \"%s\"",
+						RelationGetRelationName(relation)),
+				 errdetail("Expressions only allow columns, constants, built-in operators, built-in data types and non-immutable built-in functions.")
+				 ));
+	}
+
+	if (errdetail_msg)
+		ereport(ERROR,
+				(errmsg("invalid publication WHERE expression for relation \"%s\"",
+						RelationGetRelationName(relation)),
+				 errdetail("%s", errdetail_msg)
+				 ));
+
+	return expression_tree_walker(node, rowfilter_walker, (void *) relation);
+}
+
+/*
+ * Transform the publication WHERE clause for all the relations in list,
+ * ensuring it is coerced to boolean and necessary collation information is
+ * added if required, and add a new nsitem/RTE for the associated relation to
+ * the ParseState's namespace list.
+ *
+ * Also check the publication row filter expression and throw an error if
+ * anything not permitted or unexpected is encountered.
+ */
+static List *
+transformPubWhereClauses(List *tables, const char *queryString)
+{
+	ListCell *lc;
+
+	foreach(lc, tables)
+	{
+		ParseNamespaceItem *nsitem;
+		Node			   *whereclause = NULL;
+		ParseState		   *pstate = make_parsestate(NULL);
+		PublicationRelInfo *pri = (PublicationRelInfo *) lfirst(lc);
+
+		pstate->p_sourcetext = queryString;
+
+		nsitem = addRangeTableEntryForRelation(pstate, pri->relation,
+											   AccessShareLock, NULL,
+											   false, false);
+
+		addNSItemToQuery(pstate, nsitem, false, true, true);
+
+		whereclause = transformWhereClause(pstate,
+										   copyObject(pri->whereClause),
+										   EXPR_KIND_WHERE,
+										   "PUBLICATION WHERE");
+
+		/* Fix up collation information */
+		assign_expr_collations(pstate, whereclause);
+
+		/*
+		 * Walk the parse-tree of this publication row filter expression and
+		 * throw an error if anything not permitted or unexpected is
+		 * encountered.
+		 */
+		rowfilter_walker(whereclause, pri->relation);
+
+		free_parsestate(pstate);
+
+		pri->whereClause = whereclause;
+	}
+
+	return tables;
+}
+
+/*
  * Create new publication.
  */
 ObjectAddress
@@ -344,6 +531,8 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 			List	   *rels;
 
 			rels = OpenTableList(relations);
+			rels = transformPubWhereClauses(rels, pstate->p_sourcetext);
+
 			CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
 												  PUBLICATIONOBJ_TABLE);
 			PublicationAddTables(puboid, rels, true, NULL);
@@ -492,7 +681,8 @@ InvalidatePublicationRels(List *relids)
  */
 static void
 AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
-					   List *tables, List *schemaidlist)
+					   List *tables, List *schemaidlist,
+					   const char *queryString)
 {
 	List	   *rels = NIL;
 	Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup);
@@ -511,6 +701,8 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
 	{
 		List	   *schemas = NIL;
 
+		rels = transformPubWhereClauses(rels, queryString);
+
 		/*
 		 * Check if the relation is member of the existing schema in the
 		 * publication or member of the schema list specified.
@@ -529,6 +721,8 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
 		List	   *delrels = NIL;
 		ListCell   *oldlc;
 
+		rels = transformPubWhereClauses(rels, queryString);
+
 		/*
 		 * Check if the relation is member of the existing schema in the
 		 * publication or member of the schema list specified.
@@ -579,29 +773,11 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
 				 */
 				if (RelationGetRelid(newpubrel->relation) == oldrelid)
 				{
-					if (rfisnull && !newpubrel->whereClause)
+					if (equal(oldrelwhereclause, newpubrel->whereClause))
 					{
 						found = true;
 						break;
 					}
-
-					if (!rfisnull && newpubrel->whereClause)
-					{
-						ParseState *pstate = make_parsestate(NULL);
-						Node	   *whereclause;
-
-						whereclause = GetTransformedWhereClause(pstate,
-																newpubrel,
-																false);
-						if (equal(oldrelwhereclause, whereclause))
-						{
-							free_parsestate(pstate);
-							found = true;
-							break;
-						}
-
-						free_parsestate(pstate);
-					}
 				}
 			}
 
@@ -803,7 +979,8 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt)
 					errmsg("publication \"%s\" does not exist",
 						   stmt->pubname));
 
-		AlterPublicationTables(stmt, tup, relations, schemaidlist);
+		AlterPublicationTables(stmt, tup, relations, schemaidlist,
+							   pstate->p_sourcetext);
 		AlterPublicationSchemas(stmt, tup, schemaidlist);
 	}
 
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index a83ee25..3bdabe6 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -132,9 +132,6 @@ extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
 
 extern Oid	get_publication_oid(const char *pubname, bool missing_ok);
 extern char *get_publication_name(Oid pubid, bool missing_ok);
-extern Node *GetTransformedWhereClause(ParseState *pstate,
-									   PublicationRelInfo *pri,
-									   bool bfixupcollation);
 extern Oid	GetTopMostAncestorInPublication(Oid puboid, List *ancestors);
 
 #endif							/* PG_PUBLICATION_H */
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1a351dd..eea7108 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -357,8 +357,8 @@ RESET client_min_messages;
 -- fail - publication WHERE clause must be boolean
 ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (1234);
 ERROR:  argument of PUBLICATION WHERE must be type boolean, not type integer
-LINE 1: ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (...
-                                        ^
+LINE 1: ...PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (1234);
+                                                                 ^
 -- fail - aggregate functions not allowed in WHERE clause
 ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (e < AVG(e));
 ERROR:  aggregate functions are not allowed in WHERE
-- 
2.7.2.windows.1