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 --