agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
Subject: [PATCH v1 1/1] revamp row-security tracking
Date: Thu, 21 Nov 2024 11:57:43 -0600
---
src/backend/access/transam/xact.c | 6 ++
src/backend/executor/functions.c | 6 --
src/backend/optimizer/util/clauses.c | 6 ++
src/backend/rewrite/rewriteHandler.c | 77 ++++--------------------
src/backend/rewrite/rowsecurity.c | 88 +++++++++++++++++++++++++---
src/include/rewrite/rowsecurity.h | 6 +-
src/tools/pgindent/typedefs.list | 1 -
7 files changed, 109 insertions(+), 81 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b7ebcc2a55..440bf34906 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -51,6 +51,7 @@
#include "replication/origin.h"
#include "replication/snapbuild.h"
#include "replication/syncrep.h"
+#include "rewrite/rowsecurity.h"
#include "storage/condition_variable.h"
#include "storage/fd.h"
#include "storage/lmgr.h"
@@ -2473,6 +2474,7 @@ CommitTransaction(void)
AtEOXact_Snapshot(true, false);
AtEOXact_ApplyLauncher(true);
AtEOXact_LogicalRepWorkers(true);
+ AtEOXact_RowSecurity(true);
pgstat_report_xact_timestamp(0);
ResourceOwnerDelete(TopTransactionResourceOwner);
@@ -2766,6 +2768,7 @@ PrepareTransaction(void)
/* we treat PREPARE as ROLLBACK so far as waking workers goes */
AtEOXact_ApplyLauncher(false);
AtEOXact_LogicalRepWorkers(false);
+ AtEOXact_RowSecurity(true);
pgstat_report_xact_timestamp(0);
CurrentResourceOwner = NULL;
@@ -2985,6 +2988,7 @@ AbortTransaction(void)
AtEOXact_PgStat(false, is_parallel_worker);
AtEOXact_ApplyLauncher(false);
AtEOXact_LogicalRepWorkers(false);
+ AtEOXact_RowSecurity(false);
pgstat_report_xact_timestamp(0);
}
@@ -5197,6 +5201,7 @@ CommitSubTransaction(void)
AtEOSubXact_HashTables(true, s->nestingLevel);
AtEOSubXact_PgStat(true, s->nestingLevel);
AtSubCommit_Snapshot(s->nestingLevel);
+ AtEOXact_RowSecurity(true);
/*
* We need to restore the upper transaction's read-only state, in case the
@@ -5362,6 +5367,7 @@ AbortSubTransaction(void)
AtEOSubXact_HashTables(false, s->nestingLevel);
AtEOSubXact_PgStat(false, s->nestingLevel);
AtSubAbort_Snapshot(s->nestingLevel);
+ AtEOXact_RowSecurity(false);
}
/*
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 3988066330..692854e2b3 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -1972,12 +1972,6 @@ tlist_coercion_finished:
rtr->rtindex = 1;
newquery->jointree = makeFromExpr(list_make1(rtr), NULL);
- /*
- * Make sure the new query is marked as having row security if the
- * original one does.
- */
- newquery->hasRowSecurity = parse->hasRowSecurity;
-
/* Replace original query in the correct element of the query list */
lfirst(parse_cell) = newquery;
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 8e39795e24..775c79eca5 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -43,6 +43,7 @@
#include "parser/parse_func.h"
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
+#include "rewrite/rowsecurity.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
#include "utils/builtins.h"
@@ -5082,6 +5083,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
List *raw_parsetree_list;
List *querytree_list;
Query *querytree;
+ int rowsec_level;
Assert(rte->rtekind == RTE_FUNCTION);
@@ -5169,6 +5171,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
return NULL;
}
+ rowsec_level = PushRowSecurityNestLevel();
+
/*
* Make a temporary memory context, so that we don't leak all the stuff
* that parsing might create.
@@ -5333,6 +5337,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
* We must also notice if the inserted query adds a dependency on the
* calling role due to RLS quals.
*/
+ querytree->hasRowSecurity = PopRowSecurityNestLevel(false, rowsec_level);
if (querytree->hasRowSecurity)
root->glob->dependsOnRole = true;
@@ -5340,6 +5345,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
/* Here if func is not inlinable: release temp memory and return NULL */
fail:
+ PopRowSecurityNestLevel(true, rowsec_level);
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(mycxt);
error_context_stack = sqlerrcontext.previous;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 063afd4933..56c6ddf86d 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -58,12 +58,6 @@ typedef struct acquireLocksOnSubLinks_context
bool for_execute; /* AcquireRewriteLocks' forExecute param */
} acquireLocksOnSubLinks_context;
-typedef struct fireRIRonSubLink_context
-{
- List *activeRIRs;
- bool hasRowSecurity;
-} fireRIRonSubLink_context;
-
static bool acquireLocksOnSubLinks(Node *node,
acquireLocksOnSubLinks_context *context);
static Query *rewriteRuleAction(Query *parsetree,
@@ -1845,12 +1839,6 @@ ApplyRetrieveRule(Query *parsetree,
*/
rule_action = fireRIRrules(rule_action, activeRIRs);
- /*
- * Make sure the query is marked as having row security if the view query
- * does.
- */
- parsetree->hasRowSecurity |= rule_action->hasRowSecurity;
-
/*
* Now, plug the view query in as a subselect, converting the relation's
* original RTE to a subquery RTE.
@@ -1964,7 +1952,7 @@ markQueryForLocking(Query *qry, Node *jtnode,
* the SubLink's subselect link with the possibly-rewritten subquery.
*/
static bool
-fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
+fireRIRonSubLink(Node *node, List *activeRIRs)
{
if (node == NULL)
return false;
@@ -1974,12 +1962,7 @@ fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
/* Do what we came for */
sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
- context->activeRIRs);
-
- /*
- * Remember if any of the sublinks have row security.
- */
- context->hasRowSecurity |= ((Query *) sub->subselect)->hasRowSecurity;
+ activeRIRs);
/* Fall through to process lefthand args of SubLink */
}
@@ -1989,7 +1972,7 @@ fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
* subselects of subselects for us.
*/
return expression_tree_walker(node, fireRIRonSubLink,
- (void *) context);
+ (void *) activeRIRs);
}
@@ -2006,6 +1989,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
int origResultRelation = parsetree->resultRelation;
int rt_index;
ListCell *lc;
+ int rowsec_level = PushRowSecurityNestLevel();
/*
* Expand SEARCH and CYCLE clauses in CTEs.
@@ -2050,13 +2034,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
if (rte->rtekind == RTE_SUBQUERY)
{
rte->subquery = fireRIRrules(rte->subquery, activeRIRs);
-
- /*
- * While we are here, make sure the query is marked as having row
- * security if any of its subqueries do.
- */
- parsetree->hasRowSecurity |= rte->subquery->hasRowSecurity;
-
continue;
}
@@ -2170,12 +2147,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
cte->ctequery = (Node *)
fireRIRrules((Query *) cte->ctequery, activeRIRs);
-
- /*
- * While we are here, make sure the query is marked as having row
- * security if any of its CTEs do.
- */
- parsetree->hasRowSecurity |= ((Query *) cte->ctequery)->hasRowSecurity;
}
/*
@@ -2183,22 +2154,9 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
* the rtable and cteList.
*/
if (parsetree->hasSubLinks)
- {
- fireRIRonSubLink_context context;
-
- context.activeRIRs = activeRIRs;
- context.hasRowSecurity = false;
-
- query_tree_walker(parsetree, fireRIRonSubLink, (void *) &context,
+ query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
QTW_IGNORE_RC_SUBQUERIES);
- /*
- * Make sure the query is marked as having row security if any of its
- * sublinks do.
- */
- parsetree->hasRowSecurity |= context.hasRowSecurity;
- }
-
/*
* Apply any row-level security policies. We do this last because it
* requires special recursion detection if the new quals have sublink
@@ -2212,7 +2170,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
Relation rel;
List *securityQuals;
List *withCheckOptions;
- bool hasRowSecurity;
bool hasSubLinks;
++rt_index;
@@ -2230,14 +2187,13 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
*/
get_row_security_policies(parsetree, rte, rt_index,
&securityQuals, &withCheckOptions,
- &hasRowSecurity, &hasSubLinks);
+ &hasSubLinks);
if (securityQuals != NIL || withCheckOptions != NIL)
{
if (hasSubLinks)
{
acquireLocksOnSubLinks_context context;
- fireRIRonSubLink_context fire_context;
/*
* Recursively process the new quals, checking for infinite
@@ -2268,21 +2224,11 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
* Now that we have the locks on anything added by
* get_row_security_policies, fire any RIR rules for them.
*/
- fire_context.activeRIRs = activeRIRs;
- fire_context.hasRowSecurity = false;
-
expression_tree_walker((Node *) securityQuals,
- fireRIRonSubLink, (void *) &fire_context);
+ fireRIRonSubLink, (void *) activeRIRs);
expression_tree_walker((Node *) withCheckOptions,
- fireRIRonSubLink, (void *) &fire_context);
-
- /*
- * We can ignore the value of fire_context.hasRowSecurity
- * since we only reach this code in cases where hasRowSecurity
- * is already true.
- */
- Assert(hasRowSecurity);
+ fireRIRonSubLink, (void *) activeRIRs);
activeRIRs = list_delete_last(activeRIRs);
}
@@ -2301,17 +2247,16 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
}
/*
- * Make sure the query is marked correctly if row-level security
- * applies, or if the new quals had sublinks.
+ * Make sure the query is marked correctly if the new quals had
+ * sublinks.
*/
- if (hasRowSecurity)
- parsetree->hasRowSecurity = true;
if (hasSubLinks)
parsetree->hasSubLinks = true;
table_close(rel, NoLock);
}
+ parsetree->hasRowSecurity = PopRowSecurityNestLevel(false, rowsec_level);
return parsetree;
}
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 59fd305dd7..de3fb9325f 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -45,6 +45,7 @@
#include "rewrite/rewriteManip.h"
#include "rewrite/rowsecurity.h"
#include "utils/acl.h"
+#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/rls.h"
@@ -74,6 +75,11 @@ static void add_with_check_options(Relation rel,
static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
+static void SetRowSecurityForCurrentNestLevel(void);
+
+static Bitmapset *row_sec_bms;
+static int row_sec_nest_level = -1;
+
/*
* hooks to allow extensions to add their own security policies
*
@@ -90,14 +96,14 @@ row_security_policy_hook_type row_security_policy_hook_restrictive = NULL;
* Get any row security quals and WithCheckOption checks that should be
* applied to the specified RTE.
*
- * In addition, hasRowSecurity is set to true if row-level security is enabled
+ * In addition, we mark the current nest level if row security is enabled
* (even if this RTE doesn't have any row security quals), and hasSubLinks is
* set to true if any of the quals returned contain sublinks.
*/
void
get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
List **securityQuals, List **withCheckOptions,
- bool *hasRowSecurity, bool *hasSubLinks)
+ bool *hasSubLinks)
{
Oid user_id;
int rls_status;
@@ -110,7 +116,6 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
/* Defaults for the return values */
*securityQuals = NIL;
*withCheckOptions = NIL;
- *hasRowSecurity = false;
*hasSubLinks = false;
Assert(rte->rtekind == RTE_RELATION);
@@ -135,8 +140,8 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
/*
* RLS_NONE_ENV means we are not doing any RLS now, but that may change
- * with changes to the environment, so we mark it as hasRowSecurity to
- * force a re-plan when the environment changes.
+ * with changes to the environment, so we mark it as having row-security
+ * to force a re-plan when the environment changes.
*/
if (rls_status == RLS_NONE_ENV)
{
@@ -145,7 +150,7 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
* replanned if the environment changes (GUCs, role), but we are not
* adding anything here.
*/
- *hasRowSecurity = true;
+ SetRowSecurityForCurrentNestLevel();
return;
}
@@ -526,7 +531,7 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
* Mark this query as having row security, so plancache can invalidate it
* when necessary (eg: role changes)
*/
- *hasRowSecurity = true;
+ SetRowSecurityForCurrentNestLevel();
}
/*
@@ -930,3 +935,72 @@ check_role_for_policy(ArrayType *policy_roles, Oid user_id)
return false;
}
+
+/*
+ * The following functions are used to track whether any calls to
+ * get_row_security_policies() set hasRowSecurity between two points of code.
+ * This can be used to ensure that a Query is correctly marked as having
+ * row-level security dependencies without needing callers to preserve it in
+ * all cases.
+ *
+ * Before any potential calls to get_row_security_policies() that you care
+ * about, call PushRowSecurityNestLevel() to register your current level of
+ * recursion. Then, once any potential calls to get_row_security_policies()
+ * are done, call PopRowSecurityNestLevel() and save its result in your Query
+ * object's hasRowSecurity flag.
+ *
+ * The value returned by PushRowSecurityNestLevel() should be passed to the
+ * matching call to PopRowSecurityNestLevel(). This is used to verify that we
+ * aren't stranding any nest levels or popping extra ones inadvertently.
+ */
+
+int
+PushRowSecurityNestLevel(void)
+{
+ return ++row_sec_nest_level;
+}
+
+bool
+PopRowSecurityNestLevel(bool discard, int nest_level)
+{
+ bool ret = bms_is_member(row_sec_nest_level, row_sec_bms);
+
+ if (unlikely(nest_level != row_sec_nest_level))
+ elog(ERROR, "row security nest level mismatch");
+
+ if (ret)
+ {
+ MemoryContext oldcontext = MemoryContextSwitchTo(CurTransactionContext);
+
+ if (!discard && row_sec_nest_level > 0)
+ row_sec_bms = bms_add_member(row_sec_bms, row_sec_nest_level - 1);
+ row_sec_bms = bms_del_member(row_sec_bms, row_sec_nest_level);
+
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ row_sec_nest_level--;
+ return ret;
+}
+
+void
+AtEOXact_RowSecurity(bool isCommit)
+{
+ Assert(!isCommit || row_sec_nest_level == -1);
+
+ row_sec_bms = NULL;
+ row_sec_nest_level = -1;
+}
+
+static void
+SetRowSecurityForCurrentNestLevel(void)
+{
+ MemoryContext oldcontext = MemoryContextSwitchTo(CurTransactionContext);
+
+ if (unlikely(row_sec_nest_level < 0))
+ elog(ERROR, "no row security nest level was pushed");
+
+ row_sec_bms = bms_add_member(row_sec_bms, row_sec_nest_level);
+
+ MemoryContextSwitchTo(oldcontext);
+}
diff --git a/src/include/rewrite/rowsecurity.h b/src/include/rewrite/rowsecurity.h
index 1717ed4e5e..023f7f4091 100644
--- a/src/include/rewrite/rowsecurity.h
+++ b/src/include/rewrite/rowsecurity.h
@@ -44,6 +44,10 @@ extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook_restri
extern void get_row_security_policies(Query *root,
RangeTblEntry *rte, int rt_index,
List **securityQuals, List **withCheckOptions,
- bool *hasRowSecurity, bool *hasSubLinks);
+ bool *hasSubLinks);
+
+extern int PushRowSecurityNestLevel(void);
+extern bool PopRowSecurityNestLevel(bool discard, int nest_level);
+extern void AtEOXact_RowSecurity(bool isCommit);
#endif /* ROWSECURITY_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 08521d51a9..d7fd8507ba 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3475,7 +3475,6 @@ fill_string_relopt
finalize_primnode_context
find_dependent_phvs_context
find_expr_references_context
-fireRIRonSubLink_context
fix_join_expr_context
fix_scan_expr_context
fix_upper_expr_context
--
2.39.5 (Apple Git-154)
--RHDRa8cgsoq7xsOO--
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v1 1/1] revamp row-security tracking
In-Reply-To: <no-message-id-62209@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox