From 75d8d77852af358e76710d27b4019da6ec376973 Mon Sep 17 00:00:00 2001 From: jian he Date: Tue, 23 Dec 2025 09:58:44 +0800 Subject: [PATCH v2 2/3] refactoring CreatePolicy: split policy qual, check qual Currently CreatePolicy is only directly called through parser. But imagine copying policies from one table to another table (CREATE TABLE LIKE INCLUDING POLICIES) or changing a column's data type (ALTER COLUMN SET DATA TYPE). In these case, CreatePolicy will be called indirectly, either via the parser (indirectly), or by constructing a CreatePolicyStmt node from pg_policy catalog. As mentioned above, if we directly use pg_policy catalog info for constructing a new CreatePolicyStmt node, then the policy qual, with_check qual in the new node doesn't need to run parse analysis again, in fact, if we do parse analysis for transformed node again, it will fail. Another reason for refactoring is we may need to do parse analysis earlier because we can only use CreatePolicyStmt.rte, not CreatePolicyStmt.table. Overall splitting CreatePolicy qual, check qual parse analysis into a separate function will make CreatePolicy more neat. If you follow the call chain transformWhereClause -> transformExpr -> transformExprRecurse -> transformSubLink -> parse_sub_analyze, you can see that ParseState.p_rtable is not modified during transformWhereClause. Therefore, it is safe to pass a single RTE list to recordDependencyOnExpr during CreatePolicy. discussion: https://postgr.es/m/CACJufxE42vysVEDEmaoBGmGYLZTCgUAwh_h-c9dcSLDtD5jE3g@mail.gmail.com discussion: https://postgr.es/m/CACJufxFuEOB-i2z2qhyCG=dGwDf7g6Fs_o8cz=BUi76UuUFSOA@mail.gmail.com --- src/backend/commands/policy.c | 54 +++++++----------------- src/backend/parser/gram.y | 1 + src/backend/parser/parse_utilcmd.c | 68 ++++++++++++++++++++++++++++++ src/backend/tcop/utility.c | 2 +- src/include/commands/policy.h | 2 +- src/include/nodes/parsenodes.h | 1 + src/include/parser/parse_utilcmd.h | 1 + 7 files changed, 88 insertions(+), 41 deletions(-) diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index a47744962e9..a595cd937d5 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" @@ -564,9 +565,10 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id) * handles the execution of the CREATE POLICY command. * * stmt - the CreatePolicyStmt that describes the policy to create. + * queryString - the source text of the CREATE POLICY command. */ ObjectAddress -CreatePolicy(CreatePolicyStmt *stmt) +CreatePolicy(CreatePolicyStmt *stmt, const char *queryString) { Relation pg_policy_rel; Oid policy_id; @@ -576,9 +578,7 @@ CreatePolicy(CreatePolicyStmt *stmt) Datum *role_oids; int nitems = 0; ArrayType *role_ids; - ParseState *qual_pstate; - ParseState *with_check_pstate; - ParseNamespaceItem *nsitem; + List *rtable; Node *qual; Node *with_check_qual; ScanKeyData skey[2]; @@ -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)); @@ -640,35 +636,17 @@ CreatePolicy(CreatePolicyStmt *stmt) stmt->rte->rellockmode = AccessExclusiveLock; } - /* Open target_table to build quals. No additional lock is necessary. */ + rtable = list_make1(stmt->rte); + + /* do parse analysis for qual and check qual */ + transformPolicyStmt(stmt, NULL); + + qual = stmt->qual; + with_check_qual = stmt->with_check; + + /* Open target_table, No additional lock is necessary. */ target_table = relation_open(table_id, NoLock); - /* Add for the regular security quals */ - nsitem = addRangeTableEntryForRelation(qual_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); - /* Open pg_policy catalog */ pg_policy_rel = table_open(PolicyRelationId, RowExclusiveLock); @@ -735,11 +713,11 @@ CreatePolicy(CreatePolicyStmt *stmt) recordDependencyOn(&myself, &target, DEPENDENCY_AUTO); - recordDependencyOnExpr(&myself, qual, qual_pstate->p_rtable, + recordDependencyOnExpr(&myself, qual, rtable, DEPENDENCY_NORMAL); recordDependencyOnExpr(&myself, with_check_qual, - with_check_pstate->p_rtable, DEPENDENCY_NORMAL); + rtable, DEPENDENCY_NORMAL); /* Register role dependencies */ target.classId = AuthIdRelationId; @@ -760,8 +738,6 @@ CreatePolicy(CreatePolicyStmt *stmt) /* Clean up. */ heap_freetuple(policy_tuple); - free_parsestate(qual_pstate); - free_parsestate(with_check_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 f6d46f08c22..97900568464 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -6029,6 +6029,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 2b7b084f216..37f140d691d 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -3208,6 +3208,74 @@ transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) return stmt; } +/* + * transformPolicyStmt - parse analysis for CREATE POLICY + * + * To avoid race conditions, it's important that this function relies only on + * the relid on stmt->rte (and not on stmt->table) to determine the target + * relation. + */ +CreatePolicyStmt * +transformPolicyStmt(CreatePolicyStmt *stmt, const char *queryString) +{ + Relation target_table; + ParseNamespaceItem *nsitem; + ParseState *qual_pstate; + ParseState *with_check_pstate; + + /* Nothing to do if statement already transformed. */ + if (stmt->transformed) + { + Assert(stmt->rte != NULL); + return stmt; + } + + target_table = relation_open(stmt->rte->relid, NoLock); + + /* Parse the supplied clause */ + qual_pstate = make_parsestate(NULL); + with_check_pstate = make_parsestate(NULL); + + qual_pstate->p_sourcetext = queryString; + with_check_pstate->p_sourcetext = queryString; + + /* Add for the regular security quals */ + nsitem = addRangeTableEntryForRelation(qual_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); + + stmt->qual = transformWhereClause(qual_pstate, + stmt->qual, + EXPR_KIND_POLICY, + "POLICY"); + + stmt->with_check = transformWhereClause(with_check_pstate, + stmt->with_check, + EXPR_KIND_POLICY, + "POLICY"); + + /* Fix up collation information */ + assign_expr_collations(qual_pstate, stmt->qual); + assign_expr_collations(with_check_pstate, stmt->with_check); + + free_parsestate(qual_pstate); + free_parsestate(with_check_pstate); + + relation_close(target_table, 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 d18a3a60a46..de9dc267ef2 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1826,7 +1826,7 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_CreatePolicyStmt: /* CREATE POLICY */ - address = CreatePolicy((CreatePolicyStmt *) parsetree); + address = CreatePolicy(castNode(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 7acbd2bf72c..c217ec73be9 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3109,6 +3109,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 4965fac4495..916df6eabbe 100644 --- a/src/include/parser/parse_utilcmd.h +++ b/src/include/parser/parse_utilcmd.h @@ -28,6 +28,7 @@ extern IndexStmt *transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString); extern CreateStatsStmt *transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString); +extern CreatePolicyStmt *transformPolicyStmt(CreatePolicyStmt *stmt, const char *queryString); extern void transformRuleStmt(RuleStmt *stmt, const char *queryString, List **actions, Node **whereClause); extern List *transformCreateSchemaStmtElements(List *schemaElts, -- 2.34.1