From 6572bd55b0a63fe234ec454e7a13f3a373864e9e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 7 May 2024 13:35:10 -0400 Subject: [PATCH] Prevent RLS filters on ctid from breaking WHERE CURRENT OF . The executor only supports CurrentOfExpr as the sole tidqual of a TidScan plan node. tidpath.c failed to take any particular care about that, but would just take the first ctid equality qual it could find in the target relation's baserestrictinfo list. Originally that was fine because the grammar prevents any other WHERE conditions from being combined with CURRENT OF . However, if the relation has RLS visibility policies then those would get included in the list. Should such a policy include a condition on ctid, we'd typically grab the wrong qual and produce a malfunctioning plan. To fix, introduce a simplistic priority ordering scheme for which ctid equality qual to prefer. Real-world cases involving more than one such qual are so rare that it doesn't seem worth going to any great trouble to choose one over another, so I didn't work very hard; but this code could be extended in future if someone thinks differently. It's extremely difficult to think of a reasonable use-case for an RLS restriction involving ctid, and certainly we've heard no field reports of this failure. So this doesn't seem worthy of back-patching, but in the name of cleanliness let's fix it going forward. Patch by me, per report from Robert Haas. Discussion: https://postgr.es/m/3914881.1715038270@sss.pgh.pa.us --- src/backend/optimizer/path/tidpath.c | 88 +++++++++++++++-------- src/test/regress/expected/rowsecurity.out | 41 +++++++++++ src/test/regress/sql/rowsecurity.sql | 19 +++++ 3 files changed, 118 insertions(+), 30 deletions(-) diff --git a/src/backend/optimizer/path/tidpath.c b/src/backend/optimizer/path/tidpath.c index 2ae5ddfe43..eb11bc79c7 100644 --- a/src/backend/optimizer/path/tidpath.c +++ b/src/backend/optimizer/path/tidpath.c @@ -225,42 +225,37 @@ IsCurrentOfClause(RestrictInfo *rinfo, RelOptInfo *rel) } /* - * Extract a set of CTID conditions from the given RestrictInfo - * - * Returns a List of CTID qual RestrictInfos for the specified rel (with - * implicit OR semantics across the list), or NIL if there are no usable - * conditions. + * Is the RestrictInfo usable as a CTID qual for the specified rel? * * This function considers only base cases; AND/OR combination is handled - * below. Therefore the returned List never has more than one element. - * (Using a List may seem a bit weird, but it simplifies the caller.) + * below. */ -static List * -TidQualFromRestrictInfo(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel) +static bool +RestrictInfoIsTidQual(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel) { /* * We may ignore pseudoconstant clauses (they can't contain Vars, so could * not match anyway). */ if (rinfo->pseudoconstant) - return NIL; + return false; /* * If clause must wait till after some lower-security-level restriction * clause, reject it. */ if (!restriction_is_securely_promotable(rinfo, rel)) - return NIL; + return false; /* - * Check all base cases. If we get a match, return the clause. + * Check all base cases. */ if (IsTidEqualClause(rinfo, rel) || IsTidEqualAnyClause(root, rinfo, rel) || IsCurrentOfClause(rinfo, rel)) - return list_make1(rinfo); + return true; - return NIL; + return false; } /* @@ -270,12 +265,22 @@ TidQualFromRestrictInfo(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel) * implicit OR semantics across the list), or NIL if there are no usable * equality conditions. * - * This function is just concerned with handling AND/OR recursion. + * This function is mainly concerned with handling AND/OR recursion. + * However, we do have a special rule to enforce: if there is a CurrentOfExpr + * qual, we *must* return that and only that, else the executor may fail. + * Ordinarily a CurrentOfExpr would be all alone anyway because of grammar + * restrictions, but it is possible for RLS quals to appear AND'ed with it. + * It's even possible (if fairly useless) for the RLS quals to be CTID quals. + * So we must scan the whole rlist to see if there's a CurrentOfExpr. Since + * we have to do that, we also apply some very-trivial preference rules about + * which of the other possibilities should be chosen, in the unlikely event + * that there's more than one choice. */ static List * TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel) { - List *rlst = NIL; + RestrictInfo *tidclause = NULL; /* best simple CTID qual so far */ + List *orlist = NIL; /* best OR'ed CTID qual so far */ ListCell *l; foreach(l, rlist) @@ -284,6 +289,7 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel) if (restriction_is_or_clause(rinfo)) { + List *rlst = NIL; ListCell *j; /* @@ -308,7 +314,10 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel) RestrictInfo *ri = castNode(RestrictInfo, orarg); Assert(!restriction_is_or_clause(ri)); - sublist = TidQualFromRestrictInfo(root, ri, rel); + if (RestrictInfoIsTidQual(root, ri, rel)) + sublist = list_make1(ri); + else + sublist = NIL; } /* @@ -326,25 +335,44 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel) */ rlst = list_concat(rlst, sublist); } + + if (rlst) + { + /* + * Accept the OR'ed list if it's the first one, or if it's + * shorter than the previous one. + */ + if (orlist == NIL || list_length(rlst) < list_length(orlist)) + orlist = rlst; + } } else { /* Not an OR clause, so handle base cases */ - rlst = TidQualFromRestrictInfo(root, rinfo, rel); - } + if (RestrictInfoIsTidQual(root, rinfo, rel)) + { + /* We can stop immediately if it's a CurrentOfExpr */ + if (IsCurrentOfClause(rinfo, rel)) + return list_make1(rinfo); - /* - * Stop as soon as we find any usable CTID condition. In theory we - * could get CTID equality conditions from different AND'ed clauses, - * in which case we could try to pick the most efficient one. In - * practice, such usage seems very unlikely, so we don't bother; we - * just exit as soon as we find the first candidate. - */ - if (rlst) - break; + /* + * Otherwise, remember the first non-OR CTID qual. We could + * try to apply some preference order if there's more than + * one, but such usage seems very unlikely, so don't bother. + */ + if (tidclause == NULL) + tidclause = rinfo; + } + } } - return rlst; + /* + * Prefer any singleton CTID qual to an OR'ed list. Again, it seems + * unlikely to be worth thinking harder than that. + */ + if (tidclause) + return list_make1(tidclause); + return orlist; } /* @@ -405,7 +433,7 @@ BuildParameterizedTidPaths(PlannerInfo *root, RelOptInfo *rel, List *clauses) * might find a suitable ScalarArrayOpExpr in the rel's joininfo list, * but it seems unlikely to be worth expending the cycles to check. * And we definitely won't find a CurrentOfExpr here. Hence, we don't - * use TidQualFromRestrictInfo; but this must match that function + * use RestrictInfoIsTidQual; but this must match that function * otherwise. */ if (rinfo->pseudoconstant || diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 527cf7e7bf..4ccf98d8e9 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -3921,6 +3921,47 @@ SELECT * FROM current_check; (1 row) COMMIT; +-- Check that RLS filters that are tidquals don't override WHERE CURRENT OF +BEGIN; +CREATE TABLE current_check_2 (a int, b text); +INSERT INTO current_check_2 VALUES (1, 'Apple'); +ALTER TABLE current_check_2 ENABLE ROW LEVEL SECURITY; +ALTER TABLE current_check_2 FORCE ROW LEVEL SECURITY; +-- policy must accept ctid = (InvalidBlockNumber,0) since updates check it +-- before assigning a ctid to the new row +CREATE POLICY p1 ON current_check_2 AS PERMISSIVE + USING (ctid IN ('(0,1)', '(0,2)', '(4294967295,0)')); +SELECT ctid, * FROM current_check_2; + ctid | a | b +-------+---+------- + (0,1) | 1 | Apple +(1 row) + +DECLARE current_check_cursor CURSOR FOR SELECT * FROM current_check_2; +FETCH FROM current_check_cursor; + a | b +---+------- + 1 | Apple +(1 row) + +EXPLAIN (COSTS OFF) +UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor; + QUERY PLAN +---------------------------------------------------------------------------- + Update on current_check_2 + -> Tid Scan on current_check_2 + TID Cond: CURRENT OF current_check_cursor + Filter: (ctid = ANY ('{"(0,1)","(0,2)","(4294967295,0)"}'::tid[])) +(4 rows) + +UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor; +SELECT ctid, * FROM current_check_2; + ctid | a | b +-------+---+--------- + (0,2) | 1 | Manzana +(1 row) + +ROLLBACK; -- -- check pg_stats view filtering -- diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index 1d5ed0a647..3011d71b12 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -1726,6 +1726,25 @@ SELECT * FROM current_check; COMMIT; +-- Check that RLS filters that are tidquals don't override WHERE CURRENT OF +BEGIN; +CREATE TABLE current_check_2 (a int, b text); +INSERT INTO current_check_2 VALUES (1, 'Apple'); +ALTER TABLE current_check_2 ENABLE ROW LEVEL SECURITY; +ALTER TABLE current_check_2 FORCE ROW LEVEL SECURITY; +-- policy must accept ctid = (InvalidBlockNumber,0) since updates check it +-- before assigning a ctid to the new row +CREATE POLICY p1 ON current_check_2 AS PERMISSIVE + USING (ctid IN ('(0,1)', '(0,2)', '(4294967295,0)')); +SELECT ctid, * FROM current_check_2; +DECLARE current_check_cursor CURSOR FOR SELECT * FROM current_check_2; +FETCH FROM current_check_cursor; +EXPLAIN (COSTS OFF) +UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor; +UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor; +SELECT ctid, * FROM current_check_2; +ROLLBACK; + -- -- check pg_stats view filtering --