From 3a487812a6fe9aca42202318a5b05612e6cd9c45 Mon Sep 17 00:00:00 2001 From: "houzj.fnst" Date: Tue, 28 Dec 2021 17:20:06 +0800 Subject: [PATCH] refactor row filter Before this patch, we could transform the row filter expression twice when ALTER PUBLICATION ... SET TABLE WHERE (...). This patch move the node transformation to AlterPublicationTables() and CreatePublication() to 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 | 216 +++++++++++++++++++--- src/test/regress/expected/publication.out | 8 +- 3 files changed, 205 insertions(+), 215 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 649ca364cb..c65cc61a36 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" @@ -113,136 +108,6 @@ check_publication_add_schema(Oid schemaid) errdetail("Temporary schemas cannot be replicated."))); } -/* - * 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 - * non-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. @@ -406,36 +271,6 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, return result; } -/* - * 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 @@ -481,8 +316,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); @@ -522,25 +355,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; @@ -561,11 +376,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 61b0ff2c78..4936e8fed9 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" @@ -234,6 +239,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 + * non-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. */ @@ -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); @@ -506,6 +696,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup, return; rels = OpenTableList(tables); + rels = transformPubWhereClauses(rels, queryString); if (stmt->action == DEFELEM_ADD) { @@ -579,29 +770,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 +976,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/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 1a351dddb4..aff9c98cbb 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 @@ -413,7 +413,9 @@ ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (a IN (2, 4, 6)); ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl1 WHERE (ARRAY[a] <@ ARRAY[2, 4, 6]); -- fail - WHERE not allowed in DROP ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl1 WHERE (e < 27); -ERROR: cannot use a WHERE clause when removing a table from a publication +ERROR: column "e" does not exist +LINE 1: ...LICATION testpub5 DROP TABLE testpub_rf_tbl1 WHERE (e < 27); + ^ -- fail - cannot ALTER SET table which is a member of a pre-existing schema SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub6 FOR ALL TABLES IN SCHEMA testpub_rf_schema2; -- 2.18.4