From 4cbb54486832780a1454065879ba1957a3a78bee Mon Sep 17 00:00:00 2001 From: jian he Date: Sat, 28 Feb 2026 15:12:12 +0800 Subject: [PATCH v8 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 commitfest: https://commitfest.postgresql.org/patch/6055 --- src/backend/commands/tablecmds.c | 123 ++++++++++++++++++++++ src/backend/optimizer/util/var.c | 54 ++++++++++ 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, 223 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index de9ad4ab195..4b453bbca03 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. @@ -23404,6 +23406,9 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, bool have_wholerow = false; List *wholerow_idxoids = NIL; TupleConstr *constr = RelationGetDescr(rel)->constr; + List *pols = NIL; + Relation pg_policy; + Oid reltypid; /* * Loop through each CHECK constraint, see if it contain whole-row @@ -23586,4 +23591,122 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, } } } + + /* Now checking policy whole-row references */ + reltypid = get_rel_type_id(RelationGetRelid(rel)); + + pg_policy = table_open(PolicyRelationId, AccessShareLock); + + pols = GetRelPolicies(rel); + + foreach_oid(policyoid, pols) + { + SysScanDesc sscan; + HeapTuple policy_tuple; + ScanKeyData polskey[1]; + ObjectAddress pol_obj; + + 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); + + exprDatum = heap_getattr(policy_tuple, Anum_pg_policy_polqual, + RelationGetDescr(pg_policy), + &isnull); + if (!isnull) + { + exprString = TextDatumGetCString(exprDatum); + expr = (Node *) stringToNode(exprString); + pfree(exprString); + + have_wholerow = exprContainWholeRow(expr, reltypid); + + if (have_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); + + have_wholerow = exprContainWholeRow(expr, reltypid); + + if (have_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 66f5b598f0c..99cfe64bf8b 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,54 @@ 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)) + return query_tree_walker((Query *) node, exprContainWholeRow_walker, + context, 0); + + return expression_tree_walker(node, exprContainWholeRow_walker, context); +} + +/* + * exprContainWholeRow - + * + * Determine whether an expression contains whole-row Var reference, 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 3d27a019609..dadf4b1d6c7 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -197,6 +197,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 07d93e7def1..4b3e6778fd3 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2730,6 +2730,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; -- error +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 -- @@ -5198,12 +5224,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 6b3566271df..6445fb0f883 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -1219,6 +1219,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; -- error +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 77e3c04144e..3bbe8dadf35 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3592,6 +3592,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