From 575265b67813e860a382971d5e62fae9c745ba72 Mon Sep 17 00:00:00 2001 From: jian he Date: Sat, 23 May 2026 21:40:23 +0800 Subject: [PATCH v9 3/3] fix DDL wholerow referenced policies ALTER TABLE DROP COLUMN must fail if any policy's WHEN clause contains a whole-row reference. To achieve this, we record a dependency between the relation and the dependent policy in RememberWholeRowDependentForRebuilding. Later, performMultipleDeletions handles the deletion logic. ALTER COLUMN SET DATA TYPE fundamentally changes the table's row type. Rows with different column data types cannot be compared (see record_eq). Therefore, we must error out otherwise, any policy WHEN clause contains whole-row references may constantly error. 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. discussion: https://postgr.es/m/CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com commitfest: https://commitfest.postgresql.org/patch/6055 --- src/backend/commands/tablecmds.c | 130 ++++++++++++++++++++++ src/backend/optimizer/util/var.c | 56 ++++++++++ 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, 232 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index bd8e29de1b1..ed30c372239 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -790,6 +790,8 @@ static void RememberWholeRowDependentForRebuilding(AlteredTableInfo *tab, AlterT Relation rel, AttrNumber attnum, const char *colName); +static List *GetAllRelAssociatedPolicies(Relation rel); + /* ---------------------------------------------------------------- * DefineRelation * Creates a new relation. @@ -23976,6 +23978,7 @@ RememberWholeRowDependentForRebuilding(AlteredTableInfo *tab, AlterTableType sub ScanKeyData skey; Relation pg_index; Relation pg_constraint; + Relation pg_policy; SysScanDesc indscan; SysScanDesc conscan; HeapTuple constrTuple; @@ -23984,6 +23987,8 @@ RememberWholeRowDependentForRebuilding(AlteredTableInfo *tab, AlterTableType sub char *exprString = NULL; bool isnull; List *wholerow_idxoids = NIL; + List *pols = NIL; + Oid reltypid; Assert(subtype == AT_AlterColumnType || subtype == AT_DropColumn); @@ -24194,4 +24199,129 @@ RememberWholeRowDependentForRebuilding(AlteredTableInfo *tab, AlterTableType sub } } } + + /* Now checking policy whole-row references */ + reltypid = get_rel_type_id(RelationGetRelid(rel)); + + pg_policy = table_open(PolicyRelationId, AccessShareLock); + + pols = GetAllRelAssociatedPolicies(rel); + + foreach_oid(policyoid, pols) + { + SysScanDesc sscan; + HeapTuple policy_tuple; + ScanKeyData polskey[1]; + + 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); + + if (expr_contain_wholerow(expr, reltypid)) + { + ObjectAddress pol_obj; + + ObjectAddressSet(pol_obj, PolicyRelationId, policy->oid); + + /* + * The dependency between the policy and it's relation is + * DEPENDENCY_NORMAL + */ + RememberWholeRowDependent(tab, subtype, rel, attnum, + &pol_obj, 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); + + if (expr_contain_wholerow(expr, reltypid)) + { + ObjectAddress pol_obj; + + ObjectAddressSet(pol_obj, PolicyRelationId, policy->oid); + + /* + * The dependency between the policy and it's relation is + * DEPENDENCY_NORMAL + */ + RememberWholeRowDependent(tab, subtype, rel, attnum, + &pol_obj, DEPENDENCY_NORMAL); + } + } + } + systable_endscan(sscan); + } + table_close(pg_policy, AccessShareLock); +} + +/* + * GetAllRelAssociatedPolicies + * + * Returns a list of OIDs of all row-level security policies associated with the + * given relation. + */ +static List * +GetAllRelAssociatedPolicies(Relation rel) +{ + Relation depRel; + ScanKeyData key[3]; + SysScanDesc scan; + HeapTuple depTup; + List *result = NIL; + + depRel = table_open(DependRelationId, AccessShareLock); + 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); + + if (foundDep->classid == PolicyRelationId) + result = list_append_unique_oid(result, foundDep->objid); + } + systable_endscan(scan); + table_close(depRel, AccessShareLock); + + return result; } diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 907a255c36f..75f19271974 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 expr_contain_wholerow_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,56 @@ pull_varattnos_walker(Node *node, pull_varattnos_context *context) return expression_tree_walker(node, pull_varattnos_walker, context); } +static bool +expr_contain_wholerow_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, expr_contain_wholerow_walker, + context, 0); + + return expression_tree_walker(node, expr_contain_wholerow_walker, context); +} + +/* + * expr_contain_wholerow - + * + * 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 expr_contain_wholerow to check whole-row var existsence when in doubt. + */ +bool +expr_contain_wholerow(Node *node, Oid reltypid) +{ + contain_wholerow_context context; + + context.reltypid = reltypid; + + Assert(OidIsValid(reltypid)); + + return query_or_expression_tree_walker(node, + expr_contain_wholerow_walker, + &context, + 0); +} + + /* * pull_vars_of_level diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h index cb6241e2bdd..0491059c7cc 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -206,6 +206,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 expr_contain_wholerow(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 3a5e82c35bd..6b6fe994033 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2731,6 +2731,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 -- @@ -5199,12 +5225,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..32f2dda73a4 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 8cf40c87043..9236525b44d 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3692,6 +3692,7 @@ config_handle config_var_value 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