WHERE CURRENT OF with RLS quals that are ctid conditions

Поиск
Список
Период
Сортировка
От Tom Lane
Тема WHERE CURRENT OF with RLS quals that are ctid conditions
Дата
Msg-id 3914881.1715038270@sss.pgh.pa.us
обсуждение исходный текст
Ответы Re: WHERE CURRENT OF with RLS quals that are ctid conditions  (Robert Haas <robertmhaas@gmail.com>)
Список pgsql-hackers
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
 --

В списке pgsql-hackers по дате отправления:

Предыдущее
От: David Rowley
Дата:
Сообщение: Re: Incorrect explain output for updates/delete operations with returning-list on partitioned tables
Следующее
От: Michael Paquier
Дата:
Сообщение: Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?