From 3d50beeef17915e715aed4d1dab2e0c250c1387d Mon Sep 17 00:00:00 2001 From: jian he Date: Mon, 15 Sep 2025 11:59:43 +0800 Subject: [PATCH v1 1/2] refactor CreatePolicy discussion: https://postgr.es/m/ --- src/backend/commands/policy.c | 53 +++++++------------- src/backend/parser/gram.y | 1 + src/backend/parser/parse_utilcmd.c | 59 +++++++++++++++++++++++ src/backend/tcop/utility.c | 2 +- src/include/commands/policy.h | 2 +- src/include/nodes/parsenodes.h | 1 + src/include/parser/parse_utilcmd.h | 2 + src/test/regress/expected/rowsecurity.out | 2 + 8 files changed, 85 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 83056960fe4..799e1e3968a 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -33,6 +33,7 @@ #include "parser/parse_collate.h" #include "parser/parse_node.h" #include "parser/parse_relation.h" +#include "parser/parse_utilcmd.h" #include "rewrite/rewriteManip.h" #include "rewrite/rowsecurity.h" #include "utils/acl.h" @@ -566,7 +567,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id) * stmt - the CreatePolicyStmt that describes the policy to create. */ ObjectAddress -CreatePolicy(CreatePolicyStmt *stmt) +CreatePolicy(CreatePolicyStmt *stmt, const char *queryString) { Relation pg_policy_rel; Oid policy_id; @@ -576,8 +577,7 @@ CreatePolicy(CreatePolicyStmt *stmt) Datum *role_oids; int nitems = 0; ArrayType *role_ids; - ParseState *qual_pstate; - ParseState *with_check_pstate; + ParseState *pstate; ParseNamespaceItem *nsitem; Node *qual; Node *with_check_qual; @@ -615,10 +615,6 @@ CreatePolicy(CreatePolicyStmt *stmt) role_oids = policy_role_list_to_array(stmt->roles, &nitems); role_ids = construct_array_builtin(role_oids, nitems, OIDOID); - /* Parse the supplied clause */ - qual_pstate = make_parsestate(NULL); - with_check_pstate = make_parsestate(NULL); - /* zero-clear */ memset(values, 0, sizeof(values)); memset(isnull, 0, sizeof(isnull)); @@ -628,35 +624,23 @@ CreatePolicy(CreatePolicyStmt *stmt) 0, RangeVarCallbackForPolicy, stmt); + if (!stmt->transformed) + stmt = transformPolicyStmt(table_id, stmt, queryString); - /* Open target_table to build quals. No additional lock is necessary. */ + qual = stmt->qual; + with_check_qual = stmt->with_check; + + /* we'll need the pstate->rtable for recordDependencyOnExpr */ + pstate = make_parsestate(NULL); + pstate->p_sourcetext = queryString; + + /* No additional lock is necessary. */ target_table = relation_open(table_id, NoLock); - /* Add for the regular security quals */ - nsitem = addRangeTableEntryForRelation(qual_pstate, target_table, + nsitem = addRangeTableEntryForRelation(pstate, target_table, AccessShareLock, NULL, false, false); - addNSItemToQuery(qual_pstate, nsitem, false, true, true); - - /* Add for the with-check quals */ - nsitem = addRangeTableEntryForRelation(with_check_pstate, target_table, - AccessShareLock, - NULL, false, false); - addNSItemToQuery(with_check_pstate, nsitem, false, true, true); - - qual = transformWhereClause(qual_pstate, - stmt->qual, - EXPR_KIND_POLICY, - "POLICY"); - - with_check_qual = transformWhereClause(with_check_pstate, - stmt->with_check, - EXPR_KIND_POLICY, - "POLICY"); - - /* Fix up collation information */ - assign_expr_collations(qual_pstate, qual); - assign_expr_collations(with_check_pstate, with_check_qual); + addNSItemToQuery(pstate, nsitem, false, true, true); /* Open pg_policy catalog */ pg_policy_rel = table_open(PolicyRelationId, RowExclusiveLock); @@ -724,11 +708,11 @@ CreatePolicy(CreatePolicyStmt *stmt) recordDependencyOn(&myself, &target, DEPENDENCY_AUTO); - recordDependencyOnExpr(&myself, qual, qual_pstate->p_rtable, + recordDependencyOnExpr(&myself, qual, pstate->p_rtable, DEPENDENCY_NORMAL); recordDependencyOnExpr(&myself, with_check_qual, - with_check_pstate->p_rtable, DEPENDENCY_NORMAL); + pstate->p_rtable, DEPENDENCY_NORMAL); /* Register role dependencies */ target.classId = AuthIdRelationId; @@ -749,8 +733,7 @@ CreatePolicy(CreatePolicyStmt *stmt) /* Clean up. */ heap_freetuple(policy_tuple); - free_parsestate(qual_pstate); - free_parsestate(with_check_pstate); + free_parsestate(pstate); systable_endscan(sscan); relation_close(target_table, NoLock); table_close(pg_policy_rel, RowExclusiveLock); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 9fd48acb1f8..8016c58b49c 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -5948,6 +5948,7 @@ CreatePolicyStmt: n->roles = $8; n->qual = $9; n->with_check = $10; + n->transformed = false; $$ = (Node *) n; } ; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index e96b38a59d5..394a037e817 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -3205,6 +3205,65 @@ transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) return stmt; } +/* + * transformPolicyStmt - parse analysis for CREATE POLICY + * mainly parse analysis for qual and check qual of the policy. + * + * To avoid race conditions, it's important that this function relies only on + * the passed-in relid (and not on stmt->table) to determine the target + * relation. + */ +CreatePolicyStmt * +transformPolicyStmt(Oid relid, CreatePolicyStmt *stmt, const char *queryString) +{ + ParseState *pstate; + ParseNamespaceItem *nsitem; + Relation rel; + + /* Nothing to do if statement already transformed. */ + if (stmt->transformed) + return stmt; + + /* Set up pstate */ + pstate = make_parsestate(NULL); + pstate->p_sourcetext = queryString; + + /* + * Put the parent table into the rtable so that the expressions can refer + * to its fields without qualification. Caller is responsible for locking + * relation, but we still need to open it. + */ + rel = relation_open(relid, NoLock); + nsitem = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, + NULL, false, true); + + /* no to join list, yes to namespaces */ + addNSItemToQuery(pstate, nsitem, false, true, true); + + stmt->qual = transformWhereClause(pstate, + stmt->qual, + EXPR_KIND_POLICY, + "POLICY"); + + stmt->with_check = transformWhereClause(pstate, + stmt->with_check, + EXPR_KIND_POLICY, + "POLICY"); + /* Fix up collation information */ + assign_expr_collations(pstate, stmt->qual); + assign_expr_collations(pstate, stmt->with_check); + + free_parsestate(pstate); + + /* Close relation */ + table_close(rel, NoLock); + + /* Mark statement as successfully transformed */ + stmt->transformed = true; + + return stmt; +} /* * transformRuleStmt - diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 5f442bc3bd4..e8b3deca825 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1818,7 +1818,7 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_CreatePolicyStmt: /* CREATE POLICY */ - address = CreatePolicy((CreatePolicyStmt *) parsetree); + address = CreatePolicy((CreatePolicyStmt *) parsetree, queryString); break; case T_AlterPolicyStmt: /* ALTER POLICY */ diff --git a/src/include/commands/policy.h b/src/include/commands/policy.h index f06aa1df439..dab4030c38d 100644 --- a/src/include/commands/policy.h +++ b/src/include/commands/policy.h @@ -25,7 +25,7 @@ extern void RemovePolicyById(Oid policy_id); extern bool RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id); -extern ObjectAddress CreatePolicy(CreatePolicyStmt *stmt); +extern ObjectAddress CreatePolicy(CreatePolicyStmt *stmt, const char *queryString); extern ObjectAddress AlterPolicy(AlterPolicyStmt *stmt); extern Oid get_relation_policy_oid(Oid relid, const char *policy_name, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 86a236bd58b..a8cc660d5e6 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3068,6 +3068,7 @@ typedef struct CreatePolicyStmt List *roles; /* the roles associated with the policy */ Node *qual; /* the policy's condition */ Node *with_check; /* the policy's WITH CHECK condition. */ + bool transformed; /* true when transformPolicyStmt is finished */ } CreatePolicyStmt; /*---------------------- diff --git a/src/include/parser/parse_utilcmd.h b/src/include/parser/parse_utilcmd.h index 9f2b58de797..7a3562b88c2 100644 --- a/src/include/parser/parse_utilcmd.h +++ b/src/include/parser/parse_utilcmd.h @@ -28,6 +28,8 @@ extern IndexStmt *transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString); extern CreateStatsStmt *transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString); +extern CreatePolicyStmt *transformPolicyStmt(Oid relid, CreatePolicyStmt *stmt, + const char *queryString); extern void transformRuleStmt(RuleStmt *stmt, const char *queryString, List **actions, Node **whereClause); extern List *transformCreateSchemaStmtElements(List *schemaElts, diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 8c879509313..1dc8e5c8f42 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -4084,6 +4084,8 @@ BEGIN; CREATE TABLE t (c) AS VALUES ('bar'::text); CREATE POLICY p ON t USING (max(c)); -- fails: aggregate functions are not allowed in policy expressions ERROR: aggregate functions are not allowed in policy expressions +LINE 1: CREATE POLICY p ON t USING (max(c)); + ^ ROLLBACK; -- -- Non-target relations are only subject to SELECT policies -- 2.34.1