On Fri, Sep 12, 2025 at 4:19 PM jian he <[email protected]> wrote: > > hi. > > in [1], > RememberAllDependentForRebuilding > /* > * A policy can depend on a column because the column is > * specified in the policy's USING or WITH CHECK qual > * expressions. It might be possible to rewrite and recheck > * the policy expression, but punt for now. It's certainly > * easy enough to remove and recreate the policy; still, FIXME > * someday. > */ > After 11 year, I am trying to allow column type changes to cope with > security policy dependencies. > > CREATE TABLE s (a int, b int); > CREATE POLICY p2 ON s USING (s.b = 1); > --master branch will result error > ALTER TABLE s ALTER COLUMN b SET DATA TYPE INT8; > ERROR: cannot alter type of a column used in a policy definition > DETAIL: policy p2 on table s depends on column "b" > > with the attached patch, ALTER TABLE SET DATA TYPE can cope with columns that > have associated security policy. > The above ALTER TABLE SET DATA TYPE will just work fine. > The code roughly follows how statistics are recreated after a column > data type change.
v1 coding is not as aligned as with how statistics are recreated after data changes. For example, we have transformStatsStmt, but don't have transformPolicyStmt. v2-0001 refactor CreatePolicy. introduce transformPolicyStmt, very similar to transformStatsStmt, and we don't need two ParseState (qual_pstate, with_check_pstate). but we need one ParseState for recordDependencyOnExpr. v2-0002 is the same as v1-0001 mostly, using transformPolicyStmt in ATPostAlterTypeParse.
From 2f54d9ef0dcb62ea2ec6b10ff7f0a75ea53e795a Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Mon, 15 Sep 2025 02:29:07 +0800 Subject: [PATCH v2 2/2] let ALTER COLUMN SET DATA TYPE cope with POLICY dependency CREATE TABLE s (a int, b int); CREATE POLICY p2 ON s USING (s.b = 1); --no-error, while master branch will result error ALTER TABLE s ALTER COLUMN b SET DATA TYPE INT8; discussion: https://postgr.es/m/cacjufxe42vysvedemaobgmgylztcguawh_h-c9dcsldtd5j...@mail.gmail.com --- src/backend/commands/policy.c | 59 ++++++ src/backend/commands/tablecmds.c | 135 +++++++++++-- src/backend/parser/gram.y | 1 + src/backend/utils/adt/ruleutils.c | 189 ++++++++++++++++++ src/include/commands/policy.h | 1 + src/include/nodes/parsenodes.h | 2 + src/include/utils/ruleutils.h | 1 + .../test_ddl_deparse/test_ddl_deparse.c | 3 + src/test/regress/expected/rowsecurity.out | 50 ++++- src/test/regress/sql/rowsecurity.sql | 38 ++++ 10 files changed, 461 insertions(+), 18 deletions(-) diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 799e1e3968a..be0e4be37da 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -24,8 +24,10 @@ #include "catalog/namespace.h" #include "catalog/objectaccess.h" #include "catalog/pg_authid.h" +#include "catalog/pg_depend.h" #include "catalog/pg_policy.h" #include "catalog/pg_type.h" +#include "commands/comment.h" #include "commands/policy.h" #include "miscadmin.h" #include "nodes/pg_list.h" @@ -738,6 +740,11 @@ CreatePolicy(CreatePolicyStmt *stmt, const char *queryString) relation_close(target_table, NoLock); table_close(pg_policy_rel, RowExclusiveLock); + /* Add any requested comment */ + if (stmt->polcomment != NULL) + CreateComments(policy_id, PolicyRelationId, 0, + stmt->polcomment); + return myself; } @@ -1260,3 +1267,55 @@ relation_has_policies(Relation rel) return ret; } + +/* + * PoliciesGetRelations - + * Collect all relations this policy depends on. + * The policy's check qual or qual may reference other relations, we include + * those as well. + */ +List * +PoliciesGetRelations(Oid policyId) +{ + List *result = NIL; + Relation depRel; + ScanKeyData key[2]; + SysScanDesc depScan; + HeapTuple depTup; + + /* + * We scan pg_depend to find those things that policy being depended on. + */ + depRel = table_open(DependRelationId, AccessShareLock); + + ScanKeyInit(&key[0], + Anum_pg_depend_classid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(PolicyRelationId)); + ScanKeyInit(&key[1], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(policyId)); + + depScan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, 2, key); + while (HeapTupleIsValid(depTup = systable_getnext(depScan))) + { + Form_pg_depend pg_depend = (Form_pg_depend) GETSTRUCT(depTup); + + /* Prepend oid of the relation with this policy. */ + if (pg_depend->refclassid == RelationRelationId) + { + if (pg_depend->deptype == DEPENDENCY_AUTO) + result = lcons_oid(pg_depend->refobjid, result); + else + result = lappend_oid(result, pg_depend->refobjid); + } + } + systable_endscan(depScan); + + relation_close(depRel, AccessShareLock); + + Assert(result != NIL); + return result; +} diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3be2e051d32..0508d42ed89 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -60,6 +60,7 @@ #include "commands/comment.h" #include "commands/defrem.h" #include "commands/event_trigger.h" +#include "commands/policy.h" #include "commands/sequence.h" #include "commands/tablecmds.h" #include "commands/tablespace.h" @@ -208,6 +209,8 @@ typedef struct AlteredTableInfo char *clusterOnIndex; /* index to use for CLUSTER */ List *changedStatisticsOids; /* OIDs of statistics to rebuild */ List *changedStatisticsDefs; /* string definitions of same */ + List *changedPolicyOids; /* OIDs of policy to rebuild */ + List *changedPolicyDefs; /* string definitions of same */ } AlteredTableInfo; /* Struct describing one new constraint to check in Phase 3 scan */ @@ -546,6 +549,8 @@ static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel, IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static ObjectAddress ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, CreateStatsStmt *stmt, bool is_rebuild, LOCKMODE lockmode); +static ObjectAddress ATExecAddPolicies(AlteredTableInfo *tab, Relation rel, + CreatePolicyStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static ObjectAddress ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Constraint *newConstraint, bool recurse, bool is_readd, @@ -651,6 +656,7 @@ static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableT static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab); static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab); static void RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab); +static void RememberPolicyForRebuilding(Oid policyId, AlteredTableInfo *tab); static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode); static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, @@ -5449,6 +5455,10 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, address = ATExecAddStatistics(tab, rel, (CreateStatsStmt *) cmd->def, true, lockmode); break; + case AT_ReAddPolicies: /* ADD POLICIES */ + address = ATExecAddPolicies(tab, rel, (CreatePolicyStmt *) cmd->def, + true, lockmode); + break; case AT_AddConstraint: /* ADD CONSTRAINT */ /* Transform the command only during initial examination */ if (cur_pass == AT_PASS_ADD_CONSTR) @@ -6716,6 +6726,8 @@ alter_table_type_to_string(AlterTableType cmdtype) return "ALTER COLUMN ... DROP IDENTITY"; case AT_ReAddStatistics: return NULL; /* not real grammar */ + case AT_ReAddPolicies: + return NULL; /* not real grammar */ } return NULL; @@ -9663,6 +9675,29 @@ ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, return address; } +/* + * ALTER TABLE ADD POLICIES + * + * This is no such command in the grammar, but we use this internally to add + * AT_ReAddPolicies subcommands to rebuild policy after a table + * column type change. + */ +static ObjectAddress +ATExecAddPolicies(AlteredTableInfo *tab, Relation rel, + CreatePolicyStmt *stmt, bool is_rebuild, LOCKMODE lockmode) +{ + ObjectAddress address; + + Assert(IsA(stmt, CreatePolicyStmt)); + + /* The CreatePolicyStmt has already been through transformPolicyStmt */ + Assert(stmt->transformed); + + address = CreatePolicy(stmt, NULL); + + return address; +} + /* * ALTER TABLE ADD CONSTRAINT USING INDEX * @@ -15136,22 +15171,8 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, break; case PolicyRelationId: - - /* - * A policy can depend on a column because the column is - * specified in the policy's USING or WITH CHECK qual - * expressions. It might be possible to rewrite and recheck - * the policy expression, but punt for now. It's certainly - * easy enough to remove and recreate the policy; still, FIXME - * someday. - */ if (subtype == AT_AlterColumnType) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot alter type of a column used in a policy definition"), - errdetail("%s depends on column \"%s\"", - getObjectDescription(&foundObject, false), - colName))); + RememberPolicyForRebuilding(foundObject.objectId, tab); break; case AttrDefaultRelationId: @@ -15393,6 +15414,33 @@ RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab) } } +/* + * Subroutine for ATExecAlterColumnType: remember that a policy object needs to + * be rebuilt (which we might already know). + */ +static void +RememberPolicyForRebuilding(Oid policyId, AlteredTableInfo *tab) +{ + /* + * This de-duplication check is critical for two independent reasons: we + * mustn't try to recreate the same policy twice, and if a policy + * depends on more than one column whose type is to be altered, we must + * capture its definition string before applying any of the column type + * changes. ruleutils.c will get confused if we ask again later. + */ + if (!list_member_oid(tab->changedPolicyOids, policyId)) + { + /* OK, capture the policies's existing definition string */ + char *defstring = pg_get_policy_def_command(policyId); + + tab->changedPolicyOids = lappend_oid(tab->changedPolicyOids, + policyId); + tab->changedPolicyDefs = lappend(tab->changedPolicyDefs, + defstring); + } +} + + /* * Cleanup after we've finished all the ALTER TYPE or SET EXPRESSION * operations for a particular relation. We have to drop and recreate all the @@ -15537,6 +15585,39 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) add_exact_object_address(&obj, objects); } + /* add dependencies for new policies */ + forboth(oid_item, tab->changedPolicyOids, + def_item, tab->changedPolicyDefs) + { + Oid oldId = lfirst_oid(oid_item); + List *relids; + + relids = PoliciesGetRelations(oldId); + Assert(relids != NIL); + + /* + * As above, make sure we have lock on the relations if it's not the + * same table. However, we take AccessExclusiveLock here, aligning with + * the lock level used in CreatePolicy and RemovePolicyById. + * + * CAUTION: this should be done after all cases that grab + * AccessExclusiveLock, else we risk causing deadlock due to needing + * to promote our table lock. + */ + foreach_oid(relid, relids) + { + if (relid != tab->relid) + LockRelationOid(relid, AccessExclusiveLock); + } + + ATPostAlterTypeParse(oldId, tab->relid, InvalidOid, + (char *) lfirst(def_item), + wqueue, lockmode, tab->rewrite); + + ObjectAddressSet(obj, PolicyRelationId, oldId); + add_exact_object_address(&obj, objects); + } + /* * Queue up command to restore replica identity index marking */ @@ -15585,8 +15666,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) } /* - * Parse the previously-saved definition string for a constraint, index or - * statistics object against the newly-established column data type(s), and + * Parse the previously-saved definition string for a constraint, index, + * statistics or policy object against the newly-established column data type(s), and * queue up the resulting command parsetrees for execution. * * This might fail if, for example, you have a WHERE clause that uses an @@ -15638,6 +15719,12 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, transformStatsStmt(oldRelId, (CreateStatsStmt *) stmt, cmd)); + else if (IsA(stmt, CreatePolicyStmt)) + querytree_list = lappend(querytree_list, + transformPolicyStmt(oldRelId, + (CreatePolicyStmt *) stmt, + cmd)); + else querytree_list = lappend(querytree_list, stmt); } @@ -15788,6 +15875,20 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, tab->subcmds[AT_PASS_MISC] = lappend(tab->subcmds[AT_PASS_MISC], newcmd); } + else if (IsA(stm, CreatePolicyStmt)) + { + CreatePolicyStmt *stmt = (CreatePolicyStmt *) stm; + AlterTableCmd *newcmd; + + /* keep the policy's object's comment */ + stmt->polcomment = GetComment(oldId, PolicyRelationId, 0); + + newcmd = makeNode(AlterTableCmd); + newcmd->subtype = AT_ReAddPolicies; + newcmd->def = (Node *) stmt; + tab->subcmds[AT_PASS_MISC] = + lappend(tab->subcmds[AT_PASS_MISC], newcmd); + } else elog(ERROR, "unexpected statement type: %d", (int) nodeTag(stm)); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 8016c58b49c..1d206bc37c5 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -5942,6 +5942,7 @@ CreatePolicyStmt: CreatePolicyStmt *n = makeNode(CreatePolicyStmt); n->policy_name = $3; + n->polcomment = NULL; n->table = $5; n->permissive = $6; n->cmd_name = $7; diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 3d6e6bdbfd2..d17b4f8148c 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -33,6 +33,7 @@ #include "catalog/pg_opclass.h" #include "catalog/pg_operator.h" #include "catalog/pg_partitioned_table.h" +#include "catalog/pg_policy.h" #include "catalog/pg_proc.h" #include "catalog/pg_statistic_ext.h" #include "catalog/pg_trigger.h" @@ -57,6 +58,7 @@ #include "rewrite/rewriteHandler.h" #include "rewrite/rewriteManip.h" #include "rewrite/rewriteSupport.h" +#include "utils/acl.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/fmgroids.h" @@ -367,6 +369,7 @@ static char *pg_get_partkeydef_worker(Oid relid, int prettyFlags, bool attrsOnly, bool missing_ok); static char *pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, int prettyFlags, bool missing_ok); +static char *pg_get_policydef_worker(Oid policyId, int prettyFlags, bool missing_ok); static text *pg_get_expr_worker(text *expr, Oid relid, int prettyFlags); static int print_function_arguments(StringInfo buf, HeapTuple proctup, bool print_table_args, bool print_defaults); @@ -2611,6 +2614,192 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, return buf.data; } +/* + * Internal version that returns a full CREATE POLICY command + */ +char * +pg_get_policy_def_command(Oid policyId) +{ + int prettyFlags; + + prettyFlags = PRETTYFLAG_INDENT; + + return pg_get_policydef_worker(policyId, prettyFlags, false); +} + + +/* + * get_policy_applied_command - + * Helper function to convert pg_policy.polcmd char representation to their full + * command strings. + * Returned valid values are 'all', 'select', 'insert', 'update' and 'delete'. + */ +static char * +get_policy_applied_command(char polcmd) +{ + if (polcmd == '*') + return pstrdup("all"); + else if (polcmd == ACL_SELECT_CHR) + return pstrdup("select"); + else if (polcmd == ACL_INSERT_CHR) + return pstrdup("insert"); + else if (polcmd == ACL_UPDATE_CHR) + return pstrdup("update"); + else if (polcmd == ACL_DELETE_CHR) + return pstrdup("delete"); + else + { + elog(ERROR, "unrecognized policy command"); + return NULL; + } +} + +static char * +pg_get_policydef_worker(Oid policyId, int prettyFlags, bool missing_ok) +{ + HeapTuple tup; + Form_pg_policy policy_form; + StringInfoData buf; + SysScanDesc scandesc; + ScanKeyData scankey[1]; + Snapshot snapshot = RegisterSnapshot(GetTransactionSnapshot()); + Relation relation = table_open(PolicyRelationId, AccessShareLock); + ArrayType *policy_roles; + Datum roles_datum; + Datum datum; + bool isnull; + Oid *roles; + int num_roles; + List *context = NIL; + char *str_value; + char *exprsrc; + char *rolString; + char *policy_command; + Node *expr; + + ScanKeyInit(&scankey[0], + Anum_pg_policy_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(policyId)); + + scandesc = systable_beginscan(relation, + PolicyOidIndexId, + true, + snapshot, + 1, + scankey); + tup = systable_getnext(scandesc); + + UnregisterSnapshot(snapshot); + + if (!HeapTupleIsValid(tup)) + { + if (missing_ok) + { + systable_endscan(scandesc); + table_close(relation, AccessShareLock); + return NULL; + } + elog(ERROR, "could not find tuple for policy %u", policyId); + } + + policy_form = (Form_pg_policy) GETSTRUCT(tup); + context = deparse_context_for(get_relation_name(policy_form->polrelid), + policy_form->polrelid); + + initStringInfo(&buf); + if (OidIsValid(policy_form->oid)) + appendStringInfo(&buf, "CREATE POLICY %s ON %s ", + quote_identifier(NameStr(policy_form->polname)), + generate_qualified_relation_name(policy_form->polrelid)); + else + elog(ERROR, "invalid policy: %u", policyId); + + /* Get policy type, permissive or restrictive */ + if (policy_form->polpermissive) + appendStringInfoString(&buf, "AS PERMISSIVE "); + else + appendStringInfoString(&buf, "AS RESTRICTIVE "); + + appendStringInfoString(&buf, "FOR "); + + /* Get policy applied command type */ + policy_command = get_policy_applied_command(policy_form->polcmd); + if (strcmp(policy_command, "all") == 0) + appendStringInfoString(&buf, "ALL "); + else if (strcmp(policy_command, "select") == 0) + appendStringInfoString(&buf, "SELECT "); + else if (strcmp(policy_command, "insert") == 0) + appendStringInfoString(&buf, "INSERT "); + else if (strcmp(policy_command, "update") == 0) + appendStringInfoString(&buf, "UPDATE "); + else if (strcmp(policy_command, "delete") == 0) + appendStringInfoString(&buf, "DELETE "); + else + elog(ERROR, "invalid command type %c", policy_form->polcmd); + + appendStringInfoString(&buf, "TO "); + + /* Get the current set of roles */ + datum = heap_getattr(tup, + Anum_pg_policy_polroles, + RelationGetDescr(relation), + &isnull); + Assert(!isnull); + + policy_roles = DatumGetArrayTypePCopy(datum); + roles = (Oid *) ARR_DATA_PTR(policy_roles); + num_roles = ARR_DIMS(policy_roles)[0]; + for (int i = 0; i < num_roles; i++) + { + if (i > 0) + appendStringInfoString(&buf, ", "); + + if (OidIsValid(roles[i])) + { + datum = ObjectIdGetDatum(roles[i]); + roles_datum = DirectFunctionCall1(pg_get_userbyid, datum); + rolString = DatumGetCString(DirectFunctionCall1(nameout, roles_datum)); + appendStringInfo(&buf, "%s", rolString); + } + else + appendStringInfoString(&buf, "PUBLIC"); + } + + /* Get policy qual */ + datum = heap_getattr(tup, Anum_pg_policy_polqual, + RelationGetDescr(relation), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + expr = (Node *) stringToNode(str_value); + exprsrc = deparse_expression_pretty(expr, context, false, false, + prettyFlags, 0); + appendStringInfo(&buf, " USING (%s) ", exprsrc); + pfree(str_value); + } + + /* Get WITH CHECK qual */ + datum = heap_getattr(tup, Anum_pg_policy_polwithcheck, + RelationGetDescr(relation), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + expr = (Node *) stringToNode(str_value); + pfree(str_value); + + exprsrc = deparse_expression_pretty(expr, context, false, false, + prettyFlags, 0); + appendStringInfo(&buf, "WITH CHECK (%s)", exprsrc); + } + + /* Cleanup */ + systable_endscan(scandesc); + table_close(relation, AccessShareLock); + + return buf.data; +} + /* * Convert an int16[] Datum into a comma-separated list of column names diff --git a/src/include/commands/policy.h b/src/include/commands/policy.h index dab4030c38d..8796f302e1b 100644 --- a/src/include/commands/policy.h +++ b/src/include/commands/policy.h @@ -34,5 +34,6 @@ extern Oid get_relation_policy_oid(Oid relid, const char *policy_name, extern ObjectAddress rename_policy(RenameStmt *stmt); extern bool relation_has_policies(Relation rel); +extern List *PoliciesGetRelations(Oid policyId); #endif /* POLICY_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index a8cc660d5e6..aaf8127ed9e 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2477,6 +2477,7 @@ typedef enum AlterTableType AT_SetIdentity, /* SET identity column options */ AT_DropIdentity, /* DROP IDENTITY */ AT_ReAddStatistics, /* internal to commands/tablecmds.c */ + AT_ReAddPolicies, /* internal to commands/tablecmds.c */ } AlterTableType; typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */ @@ -3064,6 +3065,7 @@ typedef struct CreatePolicyStmt char *policy_name; /* Policy's name */ RangeVar *table; /* the table name the policy applies to */ char *cmd_name; /* the command name the policy applies to */ + char *polcomment; /* comment to apply to policies, or NULL */ bool permissive; /* restrictive or permissive policy */ List *roles; /* the roles associated with the policy */ Node *qual; /* the policy's condition */ diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h index 5f2ea2e4d0e..a82f83c6c72 100644 --- a/src/include/utils/ruleutils.h +++ b/src/include/utils/ruleutils.h @@ -34,6 +34,7 @@ extern char *pg_get_partkeydef_columns(Oid relid, bool pretty); extern char *pg_get_partconstrdef_string(Oid partitionId, char *aliasname); extern char *pg_get_constraintdef_command(Oid constraintId); +extern char *pg_get_policy_def_command(Oid policyId); extern char *deparse_expression(Node *expr, List *dpcontext, bool forceprefix, bool showimplicit); extern List *deparse_context_for(const char *aliasname, Oid relid); diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c index 193669f2bc1..f438936b719 100644 --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c @@ -308,6 +308,9 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS) case AT_ReAddStatistics: strtype = "(re) ADD STATS"; break; + case AT_ReAddPolicies: + strtype = "(re) ADD POLICIES"; + break; } if (subcmd->recurse) diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 1dc8e5c8f42..0314f851629 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -26,6 +26,25 @@ GRANT regress_rls_group2 TO regress_rls_carol; CREATE SCHEMA regress_rls_schema; GRANT ALL ON SCHEMA regress_rls_schema to public; SET search_path = regress_rls_schema; +--check alter column set data type will recreate security policy +CREATE TABLE rls_tbl (a int, b int, c int); +CREATE POLICY p1 ON rls_tbl + FOR UPDATE + TO regress_rls_alice, regress_rls_bob, regress_rls_carol + USING (a < 20) WITH CHECK (c <> 0 and a is not null); +CREATE POLICY p2 ON rls_tbl AS RESTRICTIVE + FOR ALL + TO PUBLIC + USING (a < 40) WITH CHECK (c > 0 and a is not null); +ALTER TABLE rls_tbl ALTER COLUMN a SET DATA TYPE INT8, ALTER COLUMN c SET DATA TYPE INT8; +SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'rls_tbl' ORDER BY policyname; + schemaname | tablename | policyname | permissive | roles | cmd | qual | with_check +--------------------+-----------+------------+-------------+-------------------------------------------------------+--------+----------+-------------------------------- + regress_rls_schema | rls_tbl | p1 | PERMISSIVE | {regress_rls_alice,regress_rls_bob,regress_rls_carol} | UPDATE | (a < 20) | ((c <> 0) AND (a IS NOT NULL)) + regress_rls_schema | rls_tbl | p2 | RESTRICTIVE | {public} | ALL | (a < 40) | ((c > 0) AND (a IS NOT NULL)) +(2 rows) + +DROP TABLE rls_tbl; -- setup of malicious function CREATE OR REPLACE FUNCTION f_leak(text) RETURNS bool COST 0.0000001 LANGUAGE plpgsql @@ -2357,6 +2376,33 @@ SELECT * FROM document; 14 | 11 | 1 | regress_rls_bob | new novel | (16 rows) +CREATE POLICY p5 ON document AS RESTRICTIVE FOR ALL TO regress_rls_bob, regress_rls_alice, regress_rls_carol +USING (CID IS NOT NULL AND + (WITH CTE AS (SELECT uaccount IS NOT NULL FROM uaccount) + SELECT * FROM CTE WHERE EXISTS ( + SELECT category FROM category WHERE EXISTS (SELECT uaccount FROM uaccount WHERE uaccount IS NULL)))); +COMMENT ON POLICY p5 ON document IS 'policy p5'; +CREATE TABLE pre_type_change AS SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'document'; +ALTER TABLE document ALTER COLUMN cid SET DATA TYPE INT8; +CREATE TABLE after_type_change AS SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'document'; +--should return zero row. +SELECT * FROM after_type_change +EXCEPT +SELECT * FROM after_type_change; + schemaname | tablename | policyname | permissive | roles | cmd | qual | with_check +------------+-----------+------------+------------+-------+-----+------+------------ +(0 rows) + +--comments should not change +SELECT polname, description +FROM pg_description, pg_policy c +WHERE classoid = 'pg_policy'::regclass AND objoid = c.oid AND c.polrelid = 'document'::regclass +ORDER BY polname; + polname | description +---------+------------- + p5 | policy p5 +(1 row) + -- -- ROLE/GROUP -- @@ -4824,7 +4870,7 @@ drop table rls_t, test_t; -- RESET SESSION AUTHORIZATION; DROP SCHEMA regress_rls_schema CASCADE; -NOTICE: drop cascades to 30 other objects +NOTICE: drop cascades to 32 other objects DETAIL: drop cascades to function f_leak(text) drop cascades to table uaccount drop cascades to table category @@ -4840,6 +4886,8 @@ drop cascades to table s2 drop cascades to view v2 drop cascades to table b1 drop cascades to view bv1 +drop cascades to table pre_type_change +drop cascades to table after_type_change drop cascades to table z1 drop cascades to table z2 drop cascades to table z1_blacklist diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 21ac0ca51ee..f740555fee5 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -35,6 +35,21 @@ CREATE SCHEMA regress_rls_schema; GRANT ALL ON SCHEMA regress_rls_schema to public; SET search_path = regress_rls_schema; +--check alter column set data type will recreate security policy +CREATE TABLE rls_tbl (a int, b int, c int); +CREATE POLICY p1 ON rls_tbl + FOR UPDATE + TO regress_rls_alice, regress_rls_bob, regress_rls_carol + USING (a < 20) WITH CHECK (c <> 0 and a is not null); +CREATE POLICY p2 ON rls_tbl AS RESTRICTIVE + FOR ALL + TO PUBLIC + USING (a < 40) WITH CHECK (c > 0 and a is not null); + +ALTER TABLE rls_tbl ALTER COLUMN a SET DATA TYPE INT8, ALTER COLUMN c SET DATA TYPE INT8; +SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'rls_tbl' ORDER BY policyname; +DROP TABLE rls_tbl; + -- setup of malicious function CREATE OR REPLACE FUNCTION f_leak(text) RETURNS bool COST 0.0000001 LANGUAGE plpgsql @@ -1021,6 +1036,29 @@ DROP POLICY p1 ON document; -- Just check everything went per plan SELECT * FROM document; +CREATE POLICY p5 ON document AS RESTRICTIVE FOR ALL TO regress_rls_bob, regress_rls_alice, regress_rls_carol +USING (CID IS NOT NULL AND + (WITH CTE AS (SELECT uaccount IS NOT NULL FROM uaccount) + SELECT * FROM CTE WHERE EXISTS ( + SELECT category FROM category WHERE EXISTS (SELECT uaccount FROM uaccount WHERE uaccount IS NULL)))); + +COMMENT ON POLICY p5 ON document IS 'policy p5'; + +CREATE TABLE pre_type_change AS SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'document'; +ALTER TABLE document ALTER COLUMN cid SET DATA TYPE INT8; +CREATE TABLE after_type_change AS SELECT * FROM pg_policies WHERE schemaname = 'regress_rls_schema' AND tablename = 'document'; + +--should return zero row. +SELECT * FROM after_type_change +EXCEPT +SELECT * FROM after_type_change; + +--comments should not change +SELECT polname, description +FROM pg_description, pg_policy c +WHERE classoid = 'pg_policy'::regclass AND objoid = c.oid AND c.polrelid = 'document'::regclass +ORDER BY polname; + -- -- ROLE/GROUP -- -- 2.34.1
From 777ec1a215c7e4865618f70c32dcb819192b0b0a Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Mon, 15 Sep 2025 10:49:39 +0800 Subject: [PATCH v2 1/2] refactor CreatePolicy discussion: https://postgr.es/m/cacjufxe42vysvedemaobgmgylztcguawh_h-c9dcsldtd5j...@mail.gmail.com --- 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
