Skip to content

Commit 6572bd5

Browse files
tglsfdctglsfdc
committed
Prevent RLS filters on ctid from breaking WHERE CURRENT OF <cursor>.
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 <cursor>. 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
1 parent 09fe965 commit 6572bd5

File tree

3 files changed

+118
-30
lines changed

3 files changed

+118
-30
lines changed

‎src/backend/optimizer/path/tidpath.c

Lines changed: 58 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -225,42 +225,37 @@ IsCurrentOfClause(RestrictInfo *rinfo, RelOptInfo *rel)
225225
}
226226

227227
/*
228-
* Extract a set of CTID conditions from the given RestrictInfo
229-
*
230-
* Returns a List of CTID qual RestrictInfos for the specified rel (with
231-
* implicit OR semantics across the list), or NIL if there are no usable
232-
* conditions.
228+
* Is the RestrictInfo usable as a CTID qual for the specified rel?
233229
*
234230
* This function considers only base cases; AND/OR combination is handled
235-
* below. Therefore the returned List never has more than one element.
236-
* (Using a List may seem a bit weird, but it simplifies the caller.)
231+
* below.
237232
*/
238-
static List *
239-
TidQualFromRestrictInfo(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel)
233+
static bool
234+
RestrictInfoIsTidQual(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel)
240235
{
241236
/*
242237
* We may ignore pseudoconstant clauses (they can't contain Vars, so could
243238
* not match anyway).
244239
*/
245240
if (rinfo->pseudoconstant)
246-
return NIL;
241+
return false;
247242

248243
/*
249244
* If clause must wait till after some lower-security-level restriction
250245
* clause, reject it.
251246
*/
252247
if (!restriction_is_securely_promotable(rinfo, rel))
253-
return NIL;
248+
return false;
254249

255250
/*
256-
* Check all base cases. If we get a match, return the clause.
251+
* Check all base cases.
257252
*/
258253
if (IsTidEqualClause(rinfo, rel) ||
259254
IsTidEqualAnyClause(root, rinfo, rel) ||
260255
IsCurrentOfClause(rinfo, rel))
261-
return list_make1(rinfo);
256+
return true;
262257

263-
return NIL;
258+
return false;
264259
}
265260

266261
/*
@@ -270,12 +265,22 @@ TidQualFromRestrictInfo(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel)
270265
* implicit OR semantics across the list), or NIL if there are no usable
271266
* equality conditions.
272267
*
273-
* This function is just concerned with handling AND/OR recursion.
268+
* This function is mainly concerned with handling AND/OR recursion.
269+
* However, we do have a special rule to enforce: if there is a CurrentOfExpr
270+
* qual, we *must* return that and only that, else the executor may fail.
271+
* Ordinarily a CurrentOfExpr would be all alone anyway because of grammar
272+
* restrictions, but it is possible for RLS quals to appear AND'ed with it.
273+
* It's even possible (if fairly useless) for the RLS quals to be CTID quals.
274+
* So we must scan the whole rlist to see if there's a CurrentOfExpr. Since
275+
* we have to do that, we also apply some very-trivial preference rules about
276+
* which of the other possibilities should be chosen, in the unlikely event
277+
* that there's more than one choice.
274278
*/
275279
static List *
276280
TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
277281
{
278-
List *rlst = NIL;
282+
RestrictInfo *tidclause = NULL; /* best simple CTID qual so far */
283+
List *orlist = NIL; /* best OR'ed CTID qual so far */
279284
ListCell *l;
280285

281286
foreach(l, rlist)
@@ -284,6 +289,7 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
284289

285290
if (restriction_is_or_clause(rinfo))
286291
{
292+
List *rlst = NIL;
287293
ListCell *j;
288294

289295
/*
@@ -308,7 +314,10 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
308314
RestrictInfo *ri = castNode(RestrictInfo, orarg);
309315

310316
Assert(!restriction_is_or_clause(ri));
311-
sublist = TidQualFromRestrictInfo(root, ri, rel);
317+
if (RestrictInfoIsTidQual(root, ri, rel))
318+
sublist = list_make1(ri);
319+
else
320+
sublist = NIL;
312321
}
313322

314323
/*
@@ -326,25 +335,44 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
326335
*/
327336
rlst = list_concat(rlst, sublist);
328337
}
338+
339+
if (rlst)
340+
{
341+
/*
342+
* Accept the OR'ed list if it's the first one, or if it's
343+
* shorter than the previous one.
344+
*/
345+
if (orlist == NIL || list_length(rlst) < list_length(orlist))
346+
orlist = rlst;
347+
}
329348
}
330349
else
331350
{
332351
/* Not an OR clause, so handle base cases */
333-
rlst = TidQualFromRestrictInfo(root, rinfo, rel);
334-
}
352+
if (RestrictInfoIsTidQual(root, rinfo, rel))
353+
{
354+
/* We can stop immediately if it's a CurrentOfExpr */
355+
if (IsCurrentOfClause(rinfo, rel))
356+
return list_make1(rinfo);
335357

336-
/*
337-
* Stop as soon as we find any usable CTID condition. In theory we
338-
* could get CTID equality conditions from different AND'ed clauses,
339-
* in which case we could try to pick the most efficient one. In
340-
* practice, such usage seems very unlikely, so we don't bother; we
341-
* just exit as soon as we find the first candidate.
342-
*/
343-
if (rlst)
344-
break;
358+
/*
359+
* Otherwise, remember the first non-OR CTID qual. We could
360+
* try to apply some preference order if there's more than
361+
* one, but such usage seems very unlikely, so don't bother.
362+
*/
363+
if (tidclause == NULL)
364+
tidclause = rinfo;
365+
}
366+
}
345367
}
346368

347-
return rlst;
369+
/*
370+
* Prefer any singleton CTID qual to an OR'ed list. Again, it seems
371+
* unlikely to be worth thinking harder than that.
372+
*/
373+
if (tidclause)
374+
return list_make1(tidclause);
375+
return orlist;
348376
}
349377

350378
/*
@@ -405,7 +433,7 @@ BuildParameterizedTidPaths(PlannerInfo *root, RelOptInfo *rel, List *clauses)
405433
* might find a suitable ScalarArrayOpExpr in the rel's joininfo list,
406434
* but it seems unlikely to be worth expending the cycles to check.
407435
* And we definitely won't find a CurrentOfExpr here. Hence, we don't
408-
* use TidQualFromRestrictInfo; but this must match that function
436+
* use RestrictInfoIsTidQual; but this must match that function
409437
* otherwise.
410438
*/
411439
if (rinfo->pseudoconstant ||

‎src/test/regress/expected/rowsecurity.out

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3921,6 +3921,47 @@ SELECT * FROM current_check;
39213921
(1 row)
39223922

39233923
COMMIT;
3924+
-- Check that RLS filters that are tidquals don't override WHERE CURRENT OF
3925+
BEGIN;
3926+
CREATE TABLE current_check_2 (a int, b text);
3927+
INSERT INTO current_check_2 VALUES (1, 'Apple');
3928+
ALTER TABLE current_check_2 ENABLE ROW LEVEL SECURITY;
3929+
ALTER TABLE current_check_2 FORCE ROW LEVEL SECURITY;
3930+
-- policy must accept ctid = (InvalidBlockNumber,0) since updates check it
3931+
-- before assigning a ctid to the new row
3932+
CREATE POLICY p1 ON current_check_2 AS PERMISSIVE
3933+
USING (ctid IN ('(0,1)', '(0,2)', '(4294967295,0)'));
3934+
SELECT ctid, * FROM current_check_2;
3935+
ctid | a | b
3936+
-------+---+-------
3937+
(0,1) | 1 | Apple
3938+
(1 row)
3939+
3940+
DECLARE current_check_cursor CURSOR FOR SELECT * FROM current_check_2;
3941+
FETCH FROM current_check_cursor;
3942+
a | b
3943+
---+-------
3944+
1 | Apple
3945+
(1 row)
3946+
3947+
EXPLAIN (COSTS OFF)
3948+
UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor;
3949+
QUERY PLAN
3950+
----------------------------------------------------------------------------
3951+
Update on current_check_2
3952+
-> Tid Scan on current_check_2
3953+
TID Cond: CURRENT OF current_check_cursor
3954+
Filter: (ctid = ANY ('{"(0,1)","(0,2)","(4294967295,0)"}'::tid[]))
3955+
(4 rows)
3956+
3957+
UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor;
3958+
SELECT ctid, * FROM current_check_2;
3959+
ctid | a | b
3960+
-------+---+---------
3961+
(0,2) | 1 | Manzana
3962+
(1 row)
3963+
3964+
ROLLBACK;
39243965
--
39253966
-- check pg_stats view filtering
39263967
--

‎src/test/regress/sql/rowsecurity.sql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1726,6 +1726,25 @@ SELECT * FROM current_check;
17261726

17271727
COMMIT;
17281728

1729+
-- Check that RLS filters that are tidquals don't override WHERE CURRENT OF
1730+
BEGIN;
1731+
CREATE TABLE current_check_2 (a int, b text);
1732+
INSERT INTO current_check_2 VALUES (1, 'Apple');
1733+
ALTER TABLE current_check_2 ENABLE ROW LEVEL SECURITY;
1734+
ALTER TABLE current_check_2 FORCE ROW LEVEL SECURITY;
1735+
-- policy must accept ctid = (InvalidBlockNumber,0) since updates check it
1736+
-- before assigning a ctid to the new row
1737+
CREATE POLICY p1 ON current_check_2 AS PERMISSIVE
1738+
USING (ctid IN ('(0,1)', '(0,2)', '(4294967295,0)'));
1739+
SELECT ctid, * FROM current_check_2;
1740+
DECLARE current_check_cursor CURSOR FOR SELECT * FROM current_check_2;
1741+
FETCH FROM current_check_cursor;
1742+
EXPLAIN (COSTS OFF)
1743+
UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor;
1744+
UPDATE current_check_2 SET b = 'Manzana' WHERE CURRENT OF current_check_cursor;
1745+
SELECT ctid, * FROM current_check_2;
1746+
ROLLBACK;
1747+
17291748
--
17301749
-- check pg_stats view filtering
17311750
--

0 commit comments

Comments
 (0)