From bf26465f706a4077659a9b5a04f74dcb514c2ef8 Mon Sep 17 00:00:00 2001 From: Ajin Cherian Date: Fri, 12 Nov 2021 01:19:35 -0500 Subject: [PATCH v39 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. Author: Peter Smith --- src/backend/catalog/dependency.c | 93 +++++++++++++++++++++++++++++++ src/backend/catalog/pg_publication.c | 22 +++++--- 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 | 3 +- src/test/regress/expected/publication.out | 26 ++++++--- src/test/regress/sql/publication.sql | 12 +++- 9 files changed, 154 insertions(+), 18 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index fe9c714..ba6de0a 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 { @@ -1569,6 +1575,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); +} + +/* * recordDependencyOnExpr - find expression dependencies * * This is used to find the dependencies of rules, constraint expressions, diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index eadb6d0..3a6f7a6 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -258,17 +258,25 @@ rowfilter_expr_replident_walker(Node *node, rf_context *context) /* * 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 on 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) { + char *relname = RelationGetRelationName(rel); + + /* + * Rule 1. Walk the parse-tree and reject anything unexpected. + */ + rowfilter_validator(relname, rfnode); + /* - * Rule 1: For "delete", check that filter cols are also valid replica + * Rule 2: For "delete", check that filter cols are also valid replica * identity cols. * * TODO - check later for publish "update" case. @@ -401,13 +409,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 29bebb7..212f473 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 3eca295..ab9bfe3 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, @@ -152,6 +151,8 @@ extern void performDeletion(const ObjectAddress *object, extern void performMultipleDeletions(const ObjectAddresses *objects, DropBehavior behavior, int flags); +extern void rowfilter_validator(char *relname, Node *expr); + extern void recordDependencyOnExpr(const ObjectAddress *depender, Node *expr, List *rtable, DependencyType behavior); diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 5e7fe01..1f51560 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -356,18 +356,29 @@ CREATE PUBLICATION testpub_dups FOR TABLE testpub_rf_tbl1, testpub_rf_tbl1 WHERE ERROR: conflicting or redundant row-filters for "testpub_rf_tbl1" RESET client_min_messages; -- 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 SET TABLE testpub_rf_tbl3; ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27); ERROR: invalid use of WHERE row-filter in ALTER PUBLICATION ... DROP TABLE -- fail - cannot ALTER SET table which is a member of a pre-existing schema @@ -389,6 +400,7 @@ DROP PUBLICATION testpub5; DROP PUBLICATION testpub7; 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 b127605..d59a08e 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -185,12 +185,21 @@ CREATE PUBLICATION testpub_dups FOR TABLE testpub_rf_tbl1 WHERE (a = 1), testpub CREATE PUBLICATION testpub_dups FOR TABLE testpub_rf_tbl1, testpub_rf_tbl1 WHERE (a = 2) WITH (publish="insert"); RESET client_min_messages; -- 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 SET TABLE testpub_rf_tbl3; ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27); -- fail - cannot ALTER SET table which is a member of a pre-existing schema SET client_min_messages = 'ERROR'; @@ -210,6 +219,7 @@ DROP PUBLICATION testpub5; DROP PUBLICATION testpub7; 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