From 0f4cacba6e1383d46fc32fba0a68cba23a2e3cd7 Mon Sep 17 00:00:00 2001 From: Peter Smith Date: Tue, 4 Jan 2022 15:18:24 +1100 Subject: [PATCH v58] Row Filter refactor transformations 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 moves the all the node transformation to publicationcmds.c in AlterPublicationTables() and CreatePublication() to match the division of labor, and doing so also avoids extra transformations. Also, pass the queryString to the AlterPublicationTables(), so that it can be used when transforming the expression to report correct error position. Author: Hou zj --- 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 9abade2..713dd08 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; @@ -566,11 +381,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 ab3f07f..0fce974 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); @@ -512,6 +702,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. @@ -530,6 +722,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. @@ -580,29 +774,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); - } } } @@ -805,7 +981,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 2a65204..51484b5 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 -- 1.8.3.1