Subject: WHERE CURRENT OF with RLS quals that are ctid conditions

From: Tom Lane <[email-masked]>
Date: Mon, 06 May 2024 19:31:10 -0400

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