From f438b142782374232786e12d5aca66265f48d1cb Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 27 Sep 2026 12:29:11 -0400 Subject: [PATCH v1] Prevent self-join elimination when RTEs' checkAsUser fields differ. SJE didn't consider the possibility that two RTEs referencing the same table have different securityQuals, and might choose to merge them anyway, thereby possibly losing quals that need to be enforced. This is a regression introduced by 2ebf25e7d, since before that we didn't re-generate baserestrictinfo lists from the RTEs' securityQuals after performing SJE. To defend against this, only consider SJE between RTEs with the same checkAsUser values. That solves the problem because the set of applicable RLS policies depends only on the role that is considered to be accessing the table, so that the securityQuals must be equal if the checkAsUser values are. It might also keep us from creating similar bugs if we ever invent other features that depend on the accessing role. And it's a lot cheaper than comparing securityQuals trees themselves would be, not least because we can't sort them so the preliminary sort step wouldn't help. (The first proposed solution was to not perform SJE at all on RTEs with nonempty securityQuals, but that seems rather sad, especially since such cases worked before 2ebf25e7d. Making the restriction depend on checkAsUser seems much less likely to interfere with SJE unnecessarily, since in most cases that'll be the same for all potentially-mergeable RTEs.) The added test cases show that SJE is rejected when necessary, and also demonstrate two quirks of this implementation. One is that if a removed security qual includes an InitPlan, we'll still attach the now-unused InitPlan to the plan. That's because SS_process_sublinks runs before self-join elimination, so the extra InitPlan has already been made. The other quirk is that we won't merge RTEs having zero and nonzero checkAsUser fields, even if the calling user matches the nonzero checkAsUser value so that there is no difference for this user. That's intentional so that SJE doesn't require having to mark the plan as caller-dependent. Reported-by: Yonghwa Lee Author: Tom Lane Discussion: https://postgr.es/m/20260926174457.13.noahmisch@microsoft.com Backpatch-through: 16 --- src/backend/optimizer/plan/analyzejoins.c | 47 ++++++++++++++++------- src/test/regress/expected/rowsecurity.out | 47 ++++++++++++++++++++++- src/test/regress/sql/rowsecurity.sql | 17 +++++++- 3 files changed, 95 insertions(+), 16 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index 2252185564c..1fafca57004 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -37,23 +37,27 @@ #include "optimizer/prep.h" #include "optimizer/restrictinfo.h" #include "parser/parse_agg.h" +#include "parser/parse_relation.h" #include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" /* * Utility structure. A sorting procedure is needed to simplify the search - * of SJE-candidate baserels referencing the same database relation. Having - * collected all baserels from the query jointree, the planner sorts them - * according to the reloid value, groups them with the next pass and attempts - * to remove self-joins. - * - * Preliminary sorting prevents quadratic behavior that can be harmful in the - * case of numerous joins. + * for SJE-candidate baserels, which must reference the same database relation + * with the same reader permissions (checkAsUser value). We require the + * checkAsUser fields to match to ensure that merged RTEs carry the same + * securityQuals; in future this rule might keep us out of trouble with other + * role-based features, too. Having collected all baserels from the jointree, + * remove_self_joins_recurse sorts them according to their reloid and useroid + * values, groups them in another pass and attempts to remove self-joins + * within each group. This preliminary sorting prevents quadratic behavior + * in the case of numerous joins. */ typedef struct { int relid; Oid reloid; + Oid useroid; } SelfJoinCandidate; bool enable_self_join_elimination; @@ -2011,16 +2015,27 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist) return removed; /* ... but don't fail to report sub-removals */ /* - * In order to find relations with the same oid we first build an array of - * candidates and then sort it by oid. + * In order to find relations with the same reloid/useroid we first build + * an array of candidates and then sort it by those oids. */ candidates = palloc_array(SelfJoinCandidate, numRels); i = -1; j = 0; while ((i = bms_next_member(relids, i)) >= 0) { + RangeTblEntry *rte = root->simple_rte_array[i]; + candidates[j].relid = i; - candidates[j].reloid = root->simple_rte_array[i]->relid; + candidates[j].reloid = rte->relid; + if (rte->perminfoindex != 0) + { + RTEPermissionInfo *perminfo; + + perminfo = getRTEPermissionInfo(root->parse->rteperminfos, rte); + candidates[j].useroid = perminfo->checkAsUser; + } + else + candidates[j].useroid = InvalidOid; j++; } @@ -2028,7 +2043,7 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist) self_join_candidates_cmp); /* - * Iteratively form a group of relation indexes with the same oid and + * Iteratively form a group of relation indexes with the same oids and * launch the routine that detects self-joins in this group. * * We remove considered relations from relids as we scan, so that that set @@ -2037,11 +2052,13 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist) i = 0; for (j = 1; j <= numRels; j++) { - if (j == numRels || candidates[j].reloid != candidates[i].reloid) + if (j == numRels || + candidates[j].reloid != candidates[i].reloid || + candidates[j].useroid != candidates[i].useroid) { if (j - i >= 2) { - /* Create a group of relation indexes with the same oid */ + /* Create a group of relation indexes with the same oids */ Relids group = NULL; while (i < j) @@ -2073,7 +2090,7 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist) } /* - * Compare self-join candidates by their oids. + * Compare self-join candidates by their reloid and then useroid. */ static int self_join_candidates_cmp(const void *a, const void *b) @@ -2083,6 +2100,8 @@ self_join_candidates_cmp(const void *a, const void *b) if (ca->reloid != cb->reloid) return (ca->reloid < cb->reloid ? -1 : 1); + else if (ca->useroid != cb->useroid) + return (ca->useroid < cb->useroid ? -1 : 1); else return 0; } diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 00b11544589..72d76405639 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -4869,7 +4869,7 @@ DROP USER regress_rls_dob_role2; -- Bug #15708: view + table with RLS should check policies as view owner CREATE TABLE ref_tbl (a int); INSERT INTO ref_tbl VALUES (1); -CREATE TABLE rls_tbl (a int); +CREATE TABLE rls_tbl (a int PRIMARY KEY); INSERT INTO rls_tbl VALUES (10); ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY; CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl)); @@ -4889,6 +4889,51 @@ SELECT * FROM rls_view; -- OK 10 (1 row) +EXPLAIN (COSTS OFF) -- check that RLS enforcement is actually happening +SELECT * FROM rls_view; + QUERY PLAN +------------------------------------ + Seq Scan on rls_tbl + Filter: (InitPlan exists_1).col1 + InitPlan exists_1 + -> Seq Scan on ref_tbl +(4 rows) + +-- Use the same view+table to test interaction of SJE with RLS +SET enable_self_join_elimination = on; +-- We can do SJE here, although presently an unused InitPlan survives +EXPLAIN (COSTS OFF) +SELECT * FROM rls_view r1, rls_view r2 WHERE r1.a = r2.a; + QUERY PLAN +--------------------------------------- + Seq Scan on rls_tbl + Filter: (InitPlan exists_2).col1 + InitPlan exists_1 + -> Seq Scan on ref_tbl + InitPlan exists_2 + -> Seq Scan on ref_tbl ref_tbl_1 +(6 rows) + +SET SESSION AUTHORIZATION regress_rls_bob; +-- No SJE here, because the two rls_tbl RTEs have different checkAsUser values +EXPLAIN (COSTS OFF) +SELECT * FROM rls_tbl r1, rls_view r2 WHERE r1.a = r2.a; + QUERY PLAN +------------------------------------------------ + Hash Join + Hash Cond: (r1.a = rls_tbl.a) + InitPlan exists_1 + -> Seq Scan on ref_tbl + InitPlan exists_2 + -> Seq Scan on ref_tbl ref_tbl_1 + -> Seq Scan on rls_tbl r1 + Filter: (InitPlan exists_1).col1 + -> Hash + -> Seq Scan on rls_tbl + Filter: (InitPlan exists_2).col1 +(11 rows) + +RESET enable_self_join_elimination; RESET SESSION AUTHORIZATION; DROP VIEW rls_view; DROP TABLE rls_tbl; diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 9e987b4d927..2bdb0c70c5c 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -2373,7 +2373,7 @@ DROP USER regress_rls_dob_role2; CREATE TABLE ref_tbl (a int); INSERT INTO ref_tbl VALUES (1); -CREATE TABLE rls_tbl (a int); +CREATE TABLE rls_tbl (a int PRIMARY KEY); INSERT INTO rls_tbl VALUES (10); ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY; CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl)); @@ -2386,9 +2386,24 @@ ALTER VIEW rls_view OWNER TO regress_rls_bob; GRANT SELECT ON rls_view TO regress_rls_alice; SET SESSION AUTHORIZATION regress_rls_alice; + SELECT * FROM ref_tbl; -- Permission denied SELECT * FROM rls_tbl; -- Permission denied SELECT * FROM rls_view; -- OK +EXPLAIN (COSTS OFF) -- check that RLS enforcement is actually happening +SELECT * FROM rls_view; + +-- Use the same view+table to test interaction of SJE with RLS +SET enable_self_join_elimination = on; +-- We can do SJE here, although presently an unused InitPlan survives +EXPLAIN (COSTS OFF) +SELECT * FROM rls_view r1, rls_view r2 WHERE r1.a = r2.a; +SET SESSION AUTHORIZATION regress_rls_bob; +-- No SJE here, because the two rls_tbl RTEs have different checkAsUser values +EXPLAIN (COSTS OFF) +SELECT * FROM rls_tbl r1, rls_view r2 WHERE r1.a = r2.a; + +RESET enable_self_join_elimination; RESET SESSION AUTHORIZATION; DROP VIEW rls_view; -- 2.52.0