From 5e557d6c7c15bedc086576a5fdcfc7d47c9e2c8a Mon Sep 17 00:00:00 2001 From: jian he Date: Thu, 25 Dec 2025 22:59:19 +0800 Subject: [PATCH v7 3/3] disallow ALTER TABLE ALTER COLUMN when wholerow referenced policy exists Policy have a DEPENDENCY_NORMAL type with their source table. Policy's qual and with check qual are quite unconstrained (allowing subqueries), we can't reliably use pull_varattnos to detect if they contain subqueries. A further complication is that the qual and with check qual whole-row Var may not only references their own table but also for other unrelated tables. Therefore We should check pg_depend, not pg_policy, to see if dropping this table affects any policy objects. After collecting the policies impacted by the ALTER TABLE command, check each policy qual and with check qual, see if whole-row references or not. demo: CREATE TABLE rls_tbl (a int, b int, c int); CREATE TABLE t (a int); CREATE POLICY p1 ON rls_tbl USING (rls_tbl >= ROW(1,1,1) and (select t is null from t)); ALTER TABLE t DROP COLUMN a; --error ERROR: cannot drop column a of table t because other objects depend on it DETAIL: policy p1 on table rls_tbl depends on column a of table t HINT: Use DROP ... CASCADE to drop the dependent objects too. ALTER TABLE rls_tbl DROP COLUMN b; --error ERROR: cannot drop column b of table rls_tbl because other objects depend on it DETAIL: policy p1 on table rls_tbl depends on column b of table rls_tbl HINT: Use DROP ... CASCADE to drop the dependent objects too. ALTER TABLE rls_tbl ALTER COLUMN b SET DATA TYPE BIGINT; --error ERROR: cannot alter table "rls_tbl" because security policy "p1" uses its row type HINT: You might need to drop policy "p1" first ALTER TABLE t ALTER COLUMN a SET DATA TYPE BIGINT; --error ERROR: cannot alter table "t" because security policy "p1" uses its row type HINT: You might need to drop security policy "p1" first discussion: https://postgr.es/m/CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com --- src/backend/commands/tablecmds.c | 123 ++++++++++++++++++++++ src/backend/optimizer/util/var.c | 59 +++++++++++ src/include/optimizer/optimizer.h | 1 + src/test/regress/expected/rowsecurity.out | 29 ++++- src/test/regress/sql/rowsecurity.sql | 17 +++ src/tools/pgindent/typedefs.list | 1 + 6 files changed, 228 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1e28f3af50b..95ed5e8876f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -749,6 +749,8 @@ static void recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, bool error_out); +static List *GetRelPolicies(Relation rel); + /* ---------------------------------------------------------------- * DefineRelation * Creates a new relation. @@ -23402,6 +23404,9 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, Datum exprDatum; char *exprString; bool isnull; + List *pols = NIL; + Relation pg_policy; + Oid reltypid; bool find_wholerow = false; TupleConstr *constr = RelationGetDescr(rel)->constr; @@ -23593,4 +23598,122 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, } } } + + reltypid = get_rel_type_id(RelationGetRelid(rel)); + + pg_policy = table_open(PolicyRelationId, AccessShareLock); + + pols = GetRelPolicies(rel); + + foreach_oid(policyoid, pols) + { + ObjectAddress pol_obj; + SysScanDesc sscan; + HeapTuple policy_tuple; + ScanKeyData polskey[1]; + + find_wholerow = false; + + ScanKeyInit(&polskey[0], + Anum_pg_policy_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(policyoid)); + sscan = systable_beginscan(pg_policy, + PolicyOidIndexId, true, NULL, 1, + polskey); + while (HeapTupleIsValid(policy_tuple = systable_getnext(sscan))) + { + Form_pg_policy policy = (Form_pg_policy) GETSTRUCT(policy_tuple); + + /* Get policy qual */ + exprDatum = heap_getattr(policy_tuple, Anum_pg_policy_polqual, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + exprString = TextDatumGetCString(exprDatum); + expr = (Node *) stringToNode(exprString); + pfree(exprString); + + find_wholerow = ExprContainWholeRow(expr, reltypid); + + if (find_wholerow) + { + pol_obj.classId = PolicyRelationId; + pol_obj.objectId = policy->oid; + pol_obj.objectSubId = 0; + + recordDependencyOnOrError(rel, &pol_obj, object, error_out, + DEPENDENCY_NORMAL); + + continue; + } + } + + exprDatum = heap_getattr(policy_tuple, Anum_pg_policy_polwithcheck, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + exprString = TextDatumGetCString(exprDatum); + expr = (Node *) stringToNode(exprString); + pfree(exprString); + + find_wholerow = ExprContainWholeRow(expr, reltypid); + + if (find_wholerow) + { + pol_obj.classId = PolicyRelationId; + pol_obj.objectId = policy->oid; + pol_obj.objectSubId = 0; + + recordDependencyOnOrError(rel, &pol_obj, object, error_out, + DEPENDENCY_NORMAL); + } + } + } + systable_endscan(sscan); + } + table_close(pg_policy, AccessShareLock); +} + +static List * +GetRelPolicies(Relation rel) +{ + Relation depRel; + ScanKeyData key[3]; + SysScanDesc scan; + HeapTuple depTup; + List *result = NIL; + + depRel = table_open(DependRelationId, RowExclusiveLock); + ScanKeyInit(&key[0], + Anum_pg_depend_refclassid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationRelationId)); + ScanKeyInit(&key[1], + Anum_pg_depend_refobjid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + ScanKeyInit(&key[2], + Anum_pg_depend_refobjsubid, + BTEqualStrategyNumber, F_INT4EQ, + Int32GetDatum((int32) 0)); + + scan = systable_beginscan(depRel, DependReferenceIndexId, true, + NULL, 3, key); + while (HeapTupleIsValid(depTup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(depTup); + ObjectAddress foundObject; + + foundObject.classId = foundDep->classid; + foundObject.objectId = foundDep->objid; + foundObject.objectSubId = foundDep->objsubid; + + if (foundObject.classId == PolicyRelationId) + result = list_append_unique_oid(result, foundObject.objectId); + } + systable_endscan(scan); + table_close(depRel, NoLock); + + return result; } diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 8065237a189..244c866c170 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -49,6 +49,11 @@ typedef struct int sublevels_up; } pull_vars_context; +typedef struct +{ + Oid reltypid; /* the whole-row typeid */ +} contain_wholerow_context; + typedef struct { int var_location; @@ -73,6 +78,7 @@ typedef struct static bool pull_varnos_walker(Node *node, pull_varnos_context *context); static bool pull_varattnos_walker(Node *node, pull_varattnos_context *context); +static bool ExprContainWholeRow_walker(Node *node, contain_wholerow_context *context); static bool pull_vars_walker(Node *node, pull_vars_context *context); static bool contain_var_clause_walker(Node *node, void *context); static bool contain_vars_of_level_walker(Node *node, int *sublevels_up); @@ -327,6 +333,59 @@ pull_varattnos_walker(Node *node, pull_varattnos_context *context) return expression_tree_walker(node, pull_varattnos_walker, context); } +static bool +ExprContainWholeRow_walker(Node *node, contain_wholerow_context *context) +{ + if (node == NULL) + return false; + + if (IsA(node, Var)) + { + Var *var = (Var *) node; + + if (var->varattno == InvalidAttrNumber && + var->vartype == context->reltypid) + return true; + + return false; + } + + if (IsA(node, Query)) + { + bool result; + + result = query_tree_walker((Query *) node, ExprContainWholeRow_walker, + context, 0); + return result; + } + + return expression_tree_walker(node, ExprContainWholeRow_walker, context); +} + +/* + * ExprContainWholeRow - + * + * Determine whether an expression contains a whole-row Var, recursing as needed. + * For simple expressions without sublinks, pull_varattnos is usually sufficient + * to detect a whole-row Var. But if the node contains sublinks (unplanned + * subqueries), the check must instead rely on the whole-row type OID. + * Use ExprContainWholeRow to check whole-row var existsence when in doubt. + */ +bool +ExprContainWholeRow(Node *node, Oid reltypid) +{ + contain_wholerow_context context; + + context.reltypid = reltypid; + + Assert(OidIsValid(reltypid)); + + return query_or_expression_tree_walker(node, + ExprContainWholeRow_walker, + &context, + 0); +} + /* * pull_vars_of_level diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h index 44ec5296a18..34b8e7facb7 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -199,6 +199,7 @@ extern SortGroupClause *get_sortgroupref_clause_noerr(Index sortref, extern Bitmapset *pull_varnos(PlannerInfo *root, Node *node); extern Bitmapset *pull_varnos_of_level(PlannerInfo *root, Node *node, int levelsup); extern void pull_varattnos(Node *node, Index varno, Bitmapset **varattnos); +extern bool ExprContainWholeRow(Node *node, Oid reltypid); extern List *pull_vars_of_level(Node *node, int levelsup); extern bool contain_var_clause(Node *node); extern bool contain_vars_of_level(Node *node, int levelsup); diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index c958ef4d70a..0bb74356fa7 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2637,6 +2637,32 @@ SELECT * FROM document; 14 | 11 | 1 | regress_rls_bob | new novel | (16 rows) +--check drop column (no CASCADE) or alter column data type will fail because of +--whole-row referenced security policy exists. +DROP TABLE part_document; +ALTER TABLE document ADD COLUMN dummy INT4, ADD COLUMN dummy1 INT4; +CREATE POLICY p7 ON document AS PERMISSIVE + USING (cid IS NOT NULL AND + (WITH cte AS (SELECT TRUE FROM uaccount + WHERE EXISTS (SELECT document FROM uaccount WHERE uaccount IS NULL)) + SELECT * FROM cte)); +ALTER TABLE uaccount ALTER COLUMN seclv SET DATA TYPE BIGINT; --errror +ERROR: cannot alter table "uaccount" because policy p7 on table document uses its row type +HINT: You might need to drop policy p7 on table document first +ALTER TABLE document ALTER COLUMN dummy SET DATA TYPE BIGINT; --error +ERROR: cannot alter table "document" because policy p7 on table document uses its row type +HINT: You might need to drop policy p7 on table document first +ALTER TABLE document DROP COLUMN dummy; --error +ERROR: cannot drop column dummy of table document because other objects depend on it +DETAIL: policy p7 on table document depends on column dummy of table document +HINT: Use DROP ... CASCADE to drop the dependent objects too. +ALTER TABLE uaccount DROP COLUMN seclv; --error +ERROR: cannot drop column seclv of table uaccount because other objects depend on it +DETAIL: policy p7 on table document depends on column seclv of table uaccount +HINT: Use DROP ... CASCADE to drop the dependent objects too. +ALTER TABLE document DROP COLUMN dummy CASCADE; --ok +NOTICE: drop cascades to policy p7 on table document +ALTER TABLE uaccount DROP COLUMN seclv CASCADE; --ok -- -- ROLE/GROUP -- @@ -5105,12 +5131,11 @@ 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 29 other objects DETAIL: drop cascades to function f_leak(text) drop cascades to table uaccount drop cascades to table category drop cascades to table document -drop cascades to table part_document drop cascades to table dependent drop cascades to table rec1 drop cascades to table rec2 diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 5d923c5ca3b..76487b5e4ba 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -1163,6 +1163,23 @@ DROP POLICY p1 ON document; -- Just check everything went per plan SELECT * FROM document; +--check drop column (no CASCADE) or alter column data type will fail because of +--whole-row referenced security policy exists. +DROP TABLE part_document; +ALTER TABLE document ADD COLUMN dummy INT4, ADD COLUMN dummy1 INT4; +CREATE POLICY p7 ON document AS PERMISSIVE + USING (cid IS NOT NULL AND + (WITH cte AS (SELECT TRUE FROM uaccount + WHERE EXISTS (SELECT document FROM uaccount WHERE uaccount IS NULL)) + SELECT * FROM cte)); +ALTER TABLE uaccount ALTER COLUMN seclv SET DATA TYPE BIGINT; --errror +ALTER TABLE document ALTER COLUMN dummy SET DATA TYPE BIGINT; --error + +ALTER TABLE document DROP COLUMN dummy; --error +ALTER TABLE uaccount DROP COLUMN seclv; --error +ALTER TABLE document DROP COLUMN dummy CASCADE; --ok +ALTER TABLE uaccount DROP COLUMN seclv CASCADE; --ok + -- -- ROLE/GROUP -- diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 5c88fa92f4e..a6118cc3f32 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3575,6 +3575,7 @@ conn_oauth_scope_func conn_sasl_state_func contain_aggs_of_level_context contain_placeholder_references_context +contain_wholerow_context convert_testexpr_context copy_data_dest_cb copy_data_source_cb -- 2.34.1