From 0390378c6d90c7e77cdce81e9af5b86f360354fb Mon Sep 17 00:00:00 2001 From: jian he Date: Tue, 23 Sep 2025 13:35:35 +0800 Subject: [PATCH v5 3/3] disallow change or drop column when wholerow referenced policy exists demo: CREATE TABLE rls_tbl (a int, b int, c int); CREATE POLICY p1 ON rls_tbl USING (rls_tbl >= ROW(1,1,1)); 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 rls_tbl DROP COLUMN b CASCADE; --ok NOTICE: drop cascades to policy p1 on table rls_tbl ALTER TABLE discussion: https://postgr.es/m/CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com --- src/backend/commands/tablecmds.c | 94 ++++++++++++++++++++++- src/backend/optimizer/util/var.c | 60 +++++++++++++++ src/include/optimizer/optimizer.h | 1 + src/test/regress/expected/rowsecurity.out | 27 +++++++ src/test/regress/sql/rowsecurity.sql | 17 ++++ src/tools/pgindent/typedefs.list | 1 + 6 files changed, 198 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 9bf5d6bc2b6..797cba12ab9 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -22096,8 +22096,8 @@ GetAttributeStorage(Oid atttypid, const char *storagemode) } /* - * Record dependencies between whole-row objects (indexes, CHECK constraints) - * associated with relation and relation's ObjectAddress. + * Record dependencies between whole-row objects (indexes, CHECK constraints or + * policies) associated with relation and relation's ObjectAddress. * * error_out means can not install such dependency, we have to error out explicitly. */ @@ -22107,8 +22107,13 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, boo Node *expr; List *indexlist = NIL; ObjectAddress idx_obj; + ObjectAddress pol_obj; bool find_wholerow = false; ScanKeyData skey[1]; + Relation pg_policy; + SysScanDesc sscan; + HeapTuple policy_tuple; + Oid reltypid; TupleConstr *constr = RelationGetDescr(rel)->constr; /* check CHECK constraints contain whole-row references or not */ @@ -22272,4 +22277,89 @@ recordWholeRowDependencyOnOrError(Relation rel, const ObjectAddress *object, boo } ReleaseSysCache(indexTuple); } + + /* Search pg_policy for whole-row references entries */ + find_wholerow = false; + reltypid = get_rel_type_id(RelationGetRelid(rel)); + + pg_policy = table_open(PolicyRelationId, AccessShareLock); + + ScanKeyInit(&skey[0], + Anum_pg_policy_polrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + sscan = systable_beginscan(pg_policy, + PolicyPolrelidPolnameIndexId, true, NULL, 1, + skey); + while (HeapTupleIsValid(policy_tuple = systable_getnext(sscan))) + { + Datum datum; + bool isnull; + char *str_value; + Node *polexpr; + + Form_pg_policy policy = (Form_pg_policy) GETSTRUCT(policy_tuple); + + /* Get policy qual */ + datum = heap_getattr(policy_tuple, Anum_pg_policy_polqual, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + polexpr = (Node *) stringToNode(str_value); + pfree(str_value); + + find_wholerow = ExprContainWholeRow(polexpr, reltypid); + if (find_wholerow && !error_out) + { + pol_obj.classId = PolicyRelationId; + pol_obj.objectId = policy->oid; + pol_obj.objectSubId = 0; + + /* record dependency for policies that references whole-row Var */ + recordDependencyOn(&pol_obj, object, DEPENDENCY_NORMAL); + continue; + } + + if (find_wholerow && error_out) + ereport(ERROR, + errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("cannot alter table \"%s\" because security policy \"%s\" uses its row type", + RelationGetRelationName(rel), + NameStr(policy->polname)), + errhint("You might need to drop security policy \"%s\" first", + NameStr(policy->polname))); + } + + datum = heap_getattr(policy_tuple, Anum_pg_policy_polwithcheck, + RelationGetDescr(pg_policy), &isnull); + if (!isnull) + { + str_value = TextDatumGetCString(datum); + polexpr = (Node *) stringToNode(str_value); + pfree(str_value); + + find_wholerow = ExprContainWholeRow(polexpr, reltypid); + if (find_wholerow && !error_out) + { + pol_obj.classId = PolicyRelationId; + pol_obj.objectId = policy->oid; + pol_obj.objectSubId = 0; + + /* record dependency for policies that references whole-row Var */ + recordDependencyOn(&pol_obj, object, DEPENDENCY_NORMAL); + } + + if (find_wholerow && error_out) + ereport(ERROR, + errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("cannot alter table \"%s\" because security policy \"%s\" uses its row type", + RelationGetRelationName(rel), + NameStr(policy->polname)), + errhint("You might need to drop security policy \"%s\" first", + NameStr(policy->polname))); + } + } + systable_endscan(sscan); + table_close(pg_policy, AccessShareLock); } diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 8065237a189..e5e0c4edde5 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,60 @@ 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 04878f1f1c2..69d5e6905da 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -194,6 +194,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 8c879509313..f3be08e9743 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2357,6 +2357,33 @@ 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. +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 document ALTER COLUMN dummy SET DATA TYPE BIGINT; --error +ERROR: cannot alter table "document" because security policy "p7" uses its row type +HINT: You might need to drop security policy "p7" 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 document DROP COLUMN dummy CASCADE; --ok +NOTICE: drop cascades to policy p7 on table document +CREATE POLICY p8 ON document FOR INSERT WITH CHECK (document IS NOT NULL); +ALTER TABLE document ALTER COLUMN dummy1 SET DATA TYPE BIGINT; --error +ERROR: cannot alter table "document" because security policy "p8" uses its row type +HINT: You might need to drop security policy "p8" first +ALTER TABLE document DROP COLUMN dummy1; --error +ERROR: cannot drop column dummy1 of table document because other objects depend on it +DETAIL: policy p8 on table document depends on column dummy1 of table document +HINT: Use DROP ... CASCADE to drop the dependent objects too. +ALTER TABLE document DROP COLUMN dummy1 CASCADE; --ok +NOTICE: drop cascades to policy p8 on table document -- -- ROLE/GROUP -- diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 21ac0ca51ee..ead0a5e62be 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -1021,6 +1021,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. +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 document ALTER COLUMN dummy SET DATA TYPE BIGINT; --error +ALTER TABLE document DROP COLUMN dummy; --error +ALTER TABLE document DROP COLUMN dummy CASCADE; --ok + +CREATE POLICY p8 ON document FOR INSERT WITH CHECK (document IS NOT NULL); +ALTER TABLE document ALTER COLUMN dummy1 SET DATA TYPE BIGINT; --error +ALTER TABLE document DROP COLUMN dummy1; --error +ALTER TABLE document DROP COLUMN dummy1 CASCADE; --ok + -- -- ROLE/GROUP -- diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e90af5b2ad3..3db0a0beed1 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3521,6 +3521,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