From 56992a0fc4df5c4e3f55601afc4df9853235e9ee Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 24 Dec 2025 10:47:18 +0800 Subject: [PATCH v3 3/3] ALTER TABLE command to change policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change prevents ALTER TABLE … ALTER COLUMN SET DATA TYPE from failing when the column has dependent policy objects, whereas previously it would raise an error. The approach closely mirrors how statistics are rebuilt: identify all policy objects that need to be rebuilt, construct their CREATE definitions, and then recreate those objects. However, we must fail when the qual or WITH CHECK clause contains a subquery. Otherwise, we would need to acquire AccessExclusiveLock on the referenced tables too, which seems not good, for example, changing dtaa type in table t would also lock table t1 in AccessExclusiveLock CREATE TABLE t(a int); CREATE TABLE t1(a int); CREATE POLICY p2 ON t1 AS PERMISSIVE USING ((SELECT t.a IS NOT NULL FROM t)); ALTER TABLE t ALTER COLUMN a SET DATA TYPE INT8; ERROR: cannot alter type of a column used by a policy definition when the policy contains subquery DETAIL: policy p2 on table t depends on column "a" Another reason to disallow subqueries in qual or WITH CHECK clauses is that, internally, we reconstruct policy definitions. Recreating such policies can trigger repeated RangeVar name lookups, leading to correctness and robustness issues. discussion: https://postgr.es/m/CACJufxE42vysVEDEmaoBGmGYLZTCgUAwh_h-c9dcSLDtD5jE3g@mail.gmail.com --- src/backend/commands/policy.c | 68 +++++ src/backend/commands/tablecmds.c | 246 ++++++++++++++++-- src/backend/parser/gram.y | 1 + src/backend/utils/adt/ruleutils.c | 163 ++++++++++++ src/include/commands/policy.h | 2 + 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 | 69 +++++ src/test/regress/sql/rowsecurity.sql | 54 ++++ 10 files changed, 592 insertions(+), 17 deletions(-) diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index a595cd937d5..b4a16d0ff2d 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -26,6 +26,7 @@ #include "catalog/pg_authid.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" @@ -742,6 +743,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; } @@ -1264,3 +1270,65 @@ relation_has_policies(Relation rel) return ret; } + +/* + * PolicyGetRelation: given a policy object's OID, get the OID of + * the relation it is defined on. + */ +Oid +PolicyGetRelation(Oid policyId) +{ + Relation pg_policy_rel; + ScanKeyData skey[1]; + SysScanDesc sscan; + HeapTuple tuple; + Oid relid; + + /* Fetch the policy's table OID the hard way */ + pg_policy_rel = table_open(PolicyRelationId, AccessShareLock); + ScanKeyInit(&skey[0], + Anum_pg_policy_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(policyId)); + sscan = systable_beginscan(pg_policy_rel, PolicyOidIndexId, true, + NULL, 1, skey); + tuple = systable_getnext(sscan); + + if (HeapTupleIsValid(tuple)) + relid = ((Form_pg_policy) GETSTRUCT(tuple))->polrelid; + else + { + relid = InvalidOid; /* shouldn't happen */ + elog(ERROR, "could not find tuple for policy %u", policyId); + } + systable_endscan(sscan); + + table_close(pg_policy_rel, AccessShareLock); + + return relid; +} + +/* + * get_policy_applied_command + * + * Convert pg_policy.polcmd char representation to their full command strings. + */ +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; + } +} diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6b1a00ed477..cc1cfe9bf1d 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, @@ -648,9 +653,12 @@ static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, Relation rel, AttrNumber attnum, const char *colName); +static void CheckDepentPolicies(AlteredTableInfo *tab, AlterTableType subtype, + Relation rel, AttrNumber attnum, const char *colName); 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, @@ -5464,6 +5472,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, castNode(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) @@ -6751,6 +6763,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; @@ -9723,6 +9737,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 * @@ -14868,6 +14905,13 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, */ RememberAllDependentForRebuilding(tab, AT_AlterColumnType, rel, attnum, colName); + /* + * Check that all policies does not contain subquery before adding its + * definition to tab->changedPolicyDefs, as + * RememberAllDependentForRebuilding only collects the policy OID. + */ + CheckDepentPolicies(tab, AT_AlterColumnType, rel, attnum, colName); + /* * Now scan for dependencies of this column on other things. The only * things we should find are the dependency on the column datatype and @@ -15062,6 +15106,109 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, return address; } +/* + * Subroutine for ATExecAlterColumnType. + * + * ALTER COLUMN SET DATA TYPE cannot cope with policies that contain subqueries + * in the USING or WITH CHECK qual; otherwise, repeated name lookup issues may + * occur. + */ +static void +CheckDepentPolicies(AlteredTableInfo *tab, AlterTableType subtype, + Relation rel, AttrNumber attnum, const char *colName) +{ + Relation pg_policy; + ScanKeyData skey[1]; + SysScanDesc sscan; + HeapTuple tuple; + Form_pg_policy form_policy; + Node *qual; + Node *with_check_qual; + bool hassublinks = false; + + pg_policy = table_open(PolicyRelationId, AccessShareLock); + + foreach_oid(poloid, tab->changedPolicyOids) + { + Datum datum; + bool isnull; + char *str_value; + + /* Fetch the policy's table OID the hard way */ + ScanKeyInit(&skey[0], + Anum_pg_policy_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(poloid)); + sscan = systable_beginscan(pg_policy, PolicyOidIndexId, true, + NULL, 1, skey); + tuple = systable_getnext(sscan); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "could not find tuple for pg_policy %u", poloid); + + form_policy = (Form_pg_policy) GETSTRUCT(tuple); + + /* Get policy qual */ + datum = heap_getattr(tuple, Anum_pg_policy_polqual, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + qual = stringToNode(str_value); + + if (checkExprHasSubLink(qual)) + hassublinks = true; + + pfree(str_value); + } + + if (hassublinks) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot alter type of a column used by a policy definition when the policy contains subquery"), + errdetail("policy %s on table %s depends on column \"%s\"", + NameStr(form_policy->polname), + RelationGetRelationName(rel), + colName)); + + /* Get WITH CHECK qual */ + datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + with_check_qual = stringToNode(str_value); + + if (checkExprHasSubLink(with_check_qual)) + hassublinks = true; + + pfree(str_value); + } + + if (hassublinks) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot alter type of a column used by a policy definition when the policy contains subquery"), + errdetail("policy %s on table %s depends on column \"%s\"", + NameStr(form_policy->polname), + RelationGetRelationName(rel), + colName)); + + Assert(form_policy->polrelid == RelationGetRelid(rel)); + + systable_endscan(sscan); + } + + table_close(pg_policy, AccessShareLock); + + foreach_oid(poloid, tab->changedPolicyOids) + { + /* OK, capture the policies's existing definition string */ + char *defstring = pg_get_policy_def_command(poloid); + + tab->changedPolicyDefs = lappend(tab->changedPolicyDefs, defstring); + } +} + /* * Subroutine for ATExecAlterColumnType and ATExecSetExpression: Find everything * that depends on the column (constraints, indexes, etc), and record enough @@ -15196,22 +15343,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: @@ -15453,6 +15586,27 @@ 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. + * + * changedPolicyDefs will be collected later on CheckDepentPolicies. + */ + if (!list_member_oid(tab->changedPolicyOids, policyId)) + tab->changedPolicyOids = lappend_oid(tab->changedPolicyOids, + policyId); +} + /* * 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 @@ -15597,6 +15751,35 @@ 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); + Oid relid; + + relid = PolicyGetRelation(oldId); + + /* + * 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. + */ + if (relid != tab->relid) + LockRelationOid(relid, ShareUpdateExclusiveLock); + + 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 */ @@ -15645,8 +15828,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 @@ -15698,6 +15881,21 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, transformStatsStmt(oldRelId, (CreateStatsStmt *) stmt, cmd)); + else if (IsA(stmt, CreatePolicyStmt)) + { + RangeTblEntry *rte; + CreatePolicyStmt *polstmt = castNode(CreatePolicyStmt, stmt); + + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = oldRelId; + rte->rellockmode = AccessExclusiveLock; + polstmt->rte = rte; + + querytree_list = lappend(querytree_list, + transformPolicyStmt(polstmt, + cmd)); + } else querytree_list = lappend(querytree_list, stmt); } @@ -15848,6 +16046,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 97900568464..0ec86872f16 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -6030,6 +6030,7 @@ CreatePolicyStmt: n->qual = $9; n->with_check = $10; n->transformed = false; + n->polcomment = NULL; $$ = (Node *) n; } ; diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 9f85eb86da1..a30ccf40407 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -33,11 +33,13 @@ #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" #include "catalog/pg_type.h" #include "commands/defrem.h" +#include "commands/policy.h" #include "commands/tablespace.h" #include "common/keywords.h" #include "executor/spi.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); @@ -2610,6 +2613,166 @@ 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); +} + +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 = 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 = 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..a9f6d38a0d3 100644 --- a/src/include/commands/policy.h +++ b/src/include/commands/policy.h @@ -34,5 +34,7 @@ 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 Oid PolicyGetRelation(Oid policyId); +extern char *get_policy_applied_command(char polcmd); #endif /* POLICY_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index c217ec73be9..06d9d639edf 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2508,6 +2508,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 */ @@ -3105,6 +3106,7 @@ typedef struct CreatePolicyStmt RangeTblEntry *rte; 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 7ba7d887914..b21ec468bde 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 17d72e412ff..9726fc764ee 100644 --- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c +++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c @@ -314,6 +314,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 c958ef4d70a..c748f118bda 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -26,6 +26,27 @@ 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 @@ -2637,6 +2658,54 @@ SELECT * FROM document; 14 | 11 | 1 | regress_rls_bob | new novel | (16 rows) +CREATE TABLE document_c(LIKE document INCLUDING ALL); +CREATE POLICY p5 ON document_c 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 true FROM CTE)); +ALTER TABLE document_c ALTER COLUMN cid SET DATA TYPE INT8; --error +ERROR: cannot alter type of a column used by a policy definition when the policy contains subquery +DETAIL: policy p5 on table document_c depends on column "cid" +ALTER TABLE uaccount ALTER COLUMN seclv SET DATA TYPE INT8; --error +ERROR: cannot alter type of a column used by a policy definition when the policy contains subquery +DETAIL: policy pp3 on table uaccount depends on column "seclv" +DROP POLICY p5 ON document_c CASCADE; +CREATE POLICY p5 ON document_c AS RESTRICTIVE + FOR ALL TO regress_rls_bob, regress_rls_alice, regress_rls_carol + USING (cid IS NOT NULL AND dlevel > 0); +COMMENT ON POLICY p5 ON document_c IS 'policy p5'; +SELECT * FROM pg_policies +WHERE schemaname = 'regress_rls_schema' AND tablename = 'document_c'; + schemaname | tablename | policyname | permissive | roles | cmd | qual | with_check +--------------------+------------+------------+-------------+-------------------------------------------------------+-----+--------------------------------------+------------ + regress_rls_schema | document_c | p5 | RESTRICTIVE | {regress_rls_alice,regress_rls_bob,regress_rls_carol} | ALL | ((cid IS NOT NULL) AND (dlevel > 0)) | +(1 row) + +ALTER TABLE document_c ALTER COLUMN dlevel SET DATA TYPE text; --error +ERROR: operator does not exist: text > integer +DETAIL: No operator of that name accepts the given argument types. +HINT: You might need to add explicit type casts. +ALTER TABLE document_c ALTER COLUMN cid SET DATA TYPE bool USING (cid::bool); +ALTER TABLE document_c + ALTER COLUMN cid SET DATA TYPE INT8 USING cid::INT4::INT8, + ALTER COLUMN dlevel SET DATA TYPE INT8; +SELECT * FROM pg_policies +WHERE schemaname = 'regress_rls_schema' AND tablename = 'document_c'; + schemaname | tablename | policyname | permissive | roles | cmd | qual | with_check +--------------------+------------+------------+-------------+-------------------------------------------------------+-----+--------------------------------------+------------ + regress_rls_schema | document_c | p5 | RESTRICTIVE | {regress_rls_alice,regress_rls_bob,regress_rls_carol} | ALL | ((cid IS NOT NULL) AND (dlevel > 0)) | +(1 row) + +--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_c'::regclass +ORDER BY polname; + polname | description +---------+------------- + p5 | policy p5 +(1 row) + +DROP TABLE document_c; -- -- ROLE/GROUP -- diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 5d923c5ca3b..a3a0f9e56f0 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -35,6 +35,25 @@ 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 @@ -1163,6 +1182,41 @@ DROP POLICY p1 ON document; -- Just check everything went per plan SELECT * FROM document; +CREATE TABLE document_c(LIKE document INCLUDING ALL); +CREATE POLICY p5 ON document_c 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 true FROM CTE)); + +ALTER TABLE document_c ALTER COLUMN cid SET DATA TYPE INT8; --error +ALTER TABLE uaccount ALTER COLUMN seclv SET DATA TYPE INT8; --error + +DROP POLICY p5 ON document_c CASCADE; +CREATE POLICY p5 ON document_c AS RESTRICTIVE + FOR ALL TO regress_rls_bob, regress_rls_alice, regress_rls_carol + USING (cid IS NOT NULL AND dlevel > 0); + +COMMENT ON POLICY p5 ON document_c IS 'policy p5'; + +SELECT * FROM pg_policies +WHERE schemaname = 'regress_rls_schema' AND tablename = 'document_c'; + +ALTER TABLE document_c ALTER COLUMN dlevel SET DATA TYPE text; --error +ALTER TABLE document_c ALTER COLUMN cid SET DATA TYPE bool USING (cid::bool); +ALTER TABLE document_c + ALTER COLUMN cid SET DATA TYPE INT8 USING cid::INT4::INT8, + ALTER COLUMN dlevel SET DATA TYPE INT8; + +SELECT * FROM pg_policies +WHERE schemaname = 'regress_rls_schema' AND tablename = 'document_c'; + +--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_c'::regclass +ORDER BY polname; + +DROP TABLE document_c; + -- -- ROLE/GROUP -- -- 2.34.1