Robert pointed out [1] that the planner fails if we have $SUBJECT,
because tidpath.c can seize on the RLS-derived ctid constraint
instead of the CurrentOfExpr. Since the executor can only handle
CurrentOfExpr in a TidScan's tidquals, that leads to a confusing
runtime error.
Here's a patch for that.
However ... along the way to testing it, I found that you can only
get such an RLS qual to work if it accepts "(InvalidBlockNumber,0)",
because that's what the ctid field will look like in a
not-yet-stored-to-disk tuple. That's sufficiently weird, and so
unduly in bed with undocumented implementation details, that I can't
imagine anyone is actually using such an RLS condition or ever will.
So maybe this is not really worth fixing. Thoughts?
regards, tom lane
[1]
https://www.postgresql.org/message-id/CA%2BTgmobwgL1XyV4uyUd26Nxff5WVA7%2B9XUED4yjpvft83_MBAw%40mail.gmail.com
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..a5dd726b8a 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3921,6 +3921,45 @@ 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;
+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..9bf6902c41 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1726,6 +1726,23 @@ 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;
+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
--
