Skip to content

Commit 89b280e

Browse files
tglsfdctglsfdc
committed
Fix failure in WHERE CURRENT OF after rewinding the referenced cursor.
In a case where we have multiple relation-scan nodes in a cursor plan, such as a scan of an inheritance tree, it's possible to fetch from a given scan node, then rewind the cursor and fetch some row from an earlier scan node. In such a case, execCurrent.c mistakenly thought that the later scan node was still active, because ExecReScan hadn't done anything to make it look not-active. We'd get some sort of failure in the case of a SeqScan node, because the node's scan tuple slot would be pointing at a HeapTuple whose t_self gets reset to invalid by heapam.c. But it seems possible that for other relation scan node types we'd actually return a valid tuple TID to the caller, resulting in updating or deleting a tuple that shouldn't have been considered current. To fix, forcibly clear the ScanTupleSlot in ExecScanReScan. Another issue here, which seems only latent at the moment but could easily become a live bug in future, is that rewinding a cursor does not necessarily lead to *immediately* applying ExecReScan to every scan-level node in the plan tree. Upper-level nodes will think that they can postpone that call if their child node is already marked with chgParam flags. I don't see a way for that to happen today in a plan tree that's simple enough for execCurrent.c's search_plan_tree to understand, but that's one heck of a fragile assumption. So, add some logic in search_plan_tree to detect chgParam flags being set on nodes that it descended to/through, and assume that that means we should consider lower scan nodes to be logically reset even if their ReScan call hasn't actually happened yet. Per bug #15395 from Matvey Arye. This has been broken for a long time, so back-patch to all supported branches. Discussion: https://postgr.es/m/153764171023.14986.280404050547008575@wrigleys.postgresql.org
1 parent 2f39106 commit 89b280e

File tree

4 files changed

+146
-16
lines changed

4 files changed

+146
-16
lines changed

‎src/backend/executor/execCurrent.c

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@
2323

2424

2525
static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
26-
static ScanState *search_plan_tree(PlanState *node, Oid table_oid);
26+
static ScanState *search_plan_tree(PlanState *node, Oid table_oid,
27+
bool *pending_rescan);
2728

2829

2930
/*
@@ -156,8 +157,10 @@ execCurrentOf(CurrentOfExpr *cexpr,
156157
* aggregation.
157158
*/
158159
ScanState *scanstate;
160+
bool pending_rescan = false;
159161

160-
scanstate = search_plan_tree(queryDesc->planstate, table_oid);
162+
scanstate = search_plan_tree(queryDesc->planstate, table_oid,
163+
&pending_rescan);
161164
if (!scanstate)
162165
ereport(ERROR,
163166
(errcode(ERRCODE_INVALID_CURSOR_STATE),
@@ -177,8 +180,12 @@ execCurrentOf(CurrentOfExpr *cexpr,
177180
errmsg("cursor \"%s\" is not positioned on a row",
178181
cursor_name)));
179182

180-
/* Now OK to return false if we found an inactive scan */
181-
if (TupIsNull(scanstate->ss_ScanTupleSlot))
183+
/*
184+
* Now OK to return false if we found an inactive scan. It is
185+
* inactive either if it's not positioned on a row, or there's a
186+
* rescan pending for it.
187+
*/
188+
if (TupIsNull(scanstate->ss_ScanTupleSlot) || pending_rescan)
182189
return false;
183190

184191
/*
@@ -291,10 +298,20 @@ fetch_cursor_param_value(ExprContext *econtext, int paramId)
291298
*
292299
* Search through a PlanState tree for a scan node on the specified table.
293300
* Return NULL if not found or multiple candidates.
301+
*
302+
* If a candidate is found, set *pending_rescan to true if that candidate
303+
* or any node above it has a pending rescan action, i.e. chgParam != NULL.
304+
* That indicates that we shouldn't consider the node to be positioned on a
305+
* valid tuple, even if its own state would indicate that it is. (Caller
306+
* must initialize *pending_rescan to false, and should not trust its state
307+
* if multiple candidates are found.)
294308
*/
295309
static ScanState *
296-
search_plan_tree(PlanState *node, Oid table_oid)
310+
search_plan_tree(PlanState *node, Oid table_oid,
311+
bool *pending_rescan)
297312
{
313+
ScanState *result = NULL;
314+
298315
if (node == NULL)
299316
return NULL;
300317
switch (nodeTag(node))
@@ -314,7 +331,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
314331
ScanState *sstate = (ScanState *) node;
315332

316333
if (RelationGetRelid(sstate->ss_currentRelation) == table_oid)
317-
return sstate;
334+
result = sstate;
318335
break;
319336
}
320337

@@ -325,21 +342,21 @@ search_plan_tree(PlanState *node, Oid table_oid)
325342
case T_AppendState:
326343
{
327344
AppendState *astate = (AppendState *) node;
328-
ScanState *result = NULL;
329345
int i;
330346

331347
for (i = 0; i < astate->as_nplans; i++)
332348
{
333349
ScanState *elem = search_plan_tree(astate->appendplans[i],
334-
table_oid);
350+
table_oid,
351+
pending_rescan);
335352

336353
if (!elem)
337354
continue;
338355
if (result)
339356
return NULL; /* multiple matches */
340357
result = elem;
341358
}
342-
return result;
359+
break;
343360
}
344361

345362
/*
@@ -348,21 +365,21 @@ search_plan_tree(PlanState *node, Oid table_oid)
348365
case T_MergeAppendState:
349366
{
350367
MergeAppendState *mstate = (MergeAppendState *) node;
351-
ScanState *result = NULL;
352368
int i;
353369

354370
for (i = 0; i < mstate->ms_nplans; i++)
355371
{
356372
ScanState *elem = search_plan_tree(mstate->mergeplans[i],
357-
table_oid);
373+
table_oid,
374+
pending_rescan);
358375

359376
if (!elem)
360377
continue;
361378
if (result)
362379
return NULL; /* multiple matches */
363380
result = elem;
364381
}
365-
return result;
382+
break;
366383
}
367384

368385
/*
@@ -371,18 +388,31 @@ search_plan_tree(PlanState *node, Oid table_oid)
371388
*/
372389
case T_ResultState:
373390
case T_LimitState:
374-
return search_plan_tree(node->lefttree, table_oid);
391+
result = search_plan_tree(node->lefttree,
392+
table_oid,
393+
pending_rescan);
394+
break;
375395

376396
/*
377397
* SubqueryScan too, but it keeps the child in a different place
378398
*/
379399
case T_SubqueryScanState:
380-
return search_plan_tree(((SubqueryScanState *) node)->subplan,
381-
table_oid);
400+
result = search_plan_tree(((SubqueryScanState *) node)->subplan,
401+
table_oid,
402+
pending_rescan);
403+
break;
382404

383405
default:
384406
/* Otherwise, assume we can't descend through it */
385407
break;
386408
}
387-
return NULL;
409+
410+
/*
411+
* If we found a candidate at or below this node, then this node's
412+
* chgParam indicates a pending rescan that will affect the candidate.
413+
*/
414+
if (result && node->chgParam != NULL)
415+
*pending_rescan = true;
416+
417+
return result;
388418
}

‎src/backend/executor/execScan.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,12 @@ ExecScanReScan(ScanState *node)
263263
{
264264
EState *estate = node->ps.state;
265265

266+
/*
267+
* We must clear the scan tuple so that observers (e.g., execCurrent.c)
268+
* can tell that this plan node is not positioned on a tuple.
269+
*/
270+
ExecClearTuple(node->ss_ScanTupleSlot);
271+
266272
/* Rescan EvalPlanQual tuple if we're inside an EvalPlanQual recheck */
267273
if (estate->es_epqScanDone != NULL)
268274
{

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,76 @@ SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
12691269
----------
12701270
(0 rows)
12711271

1272+
ROLLBACK;
1273+
-- Check behavior with rewinding to a previous child scan node,
1274+
-- as per bug #15395
1275+
BEGIN;
1276+
CREATE TABLE current_check (currentid int, payload text);
1277+
CREATE TABLE current_check_1 () INHERITS (current_check);
1278+
CREATE TABLE current_check_2 () INHERITS (current_check);
1279+
INSERT INTO current_check_1 SELECT i, 'p' || i FROM generate_series(1,9) i;
1280+
INSERT INTO current_check_2 SELECT i, 'P' || i FROM generate_series(10,19) i;
1281+
DECLARE c1 SCROLL CURSOR FOR SELECT * FROM current_check;
1282+
-- This tests the fetch-backwards code path
1283+
FETCH ABSOLUTE 12 FROM c1;
1284+
currentid | payload
1285+
-----------+---------
1286+
12 | P12
1287+
(1 row)
1288+
1289+
FETCH ABSOLUTE 8 FROM c1;
1290+
currentid | payload
1291+
-----------+---------
1292+
8 | p8
1293+
(1 row)
1294+
1295+
DELETE FROM current_check WHERE CURRENT OF c1 RETURNING *;
1296+
currentid | payload
1297+
-----------+---------
1298+
8 | p8
1299+
(1 row)
1300+
1301+
-- This tests the ExecutorRewind code path
1302+
FETCH ABSOLUTE 13 FROM c1;
1303+
currentid | payload
1304+
-----------+---------
1305+
13 | P13
1306+
(1 row)
1307+
1308+
FETCH ABSOLUTE 1 FROM c1;
1309+
currentid | payload
1310+
-----------+---------
1311+
1 | p1
1312+
(1 row)
1313+
1314+
DELETE FROM current_check WHERE CURRENT OF c1 RETURNING *;
1315+
currentid | payload
1316+
-----------+---------
1317+
1 | p1
1318+
(1 row)
1319+
1320+
SELECT * FROM current_check;
1321+
currentid | payload
1322+
-----------+---------
1323+
2 | p2
1324+
3 | p3
1325+
4 | p4
1326+
5 | p5
1327+
6 | p6
1328+
7 | p7
1329+
9 | p9
1330+
10 | P10
1331+
11 | P11
1332+
12 | P12
1333+
13 | P13
1334+
14 | P14
1335+
15 | P15
1336+
16 | P16
1337+
17 | P17
1338+
18 | P18
1339+
19 | P19
1340+
(17 rows)
1341+
12721342
ROLLBACK;
12731343
-- Make sure snapshot management works okay, per bug report in
12741344
-- 235395b90909301035v7228ce63q392931f15aa74b31@mail.gmail.com

‎src/test/regress/sql/portals.sql

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,30 @@ DELETE FROM onek WHERE CURRENT OF c1;
472472
SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
473473
ROLLBACK;
474474

475+
-- Check behavior with rewinding to a previous child scan node,
476+
-- as per bug #15395
477+
BEGIN;
478+
CREATE TABLE current_check (currentid int, payload text);
479+
CREATE TABLE current_check_1 () INHERITS (current_check);
480+
CREATE TABLE current_check_2 () INHERITS (current_check);
481+
INSERT INTO current_check_1 SELECT i, 'p' || i FROM generate_series(1,9) i;
482+
INSERT INTO current_check_2 SELECT i, 'P' || i FROM generate_series(10,19) i;
483+
484+
DECLARE c1 SCROLL CURSOR FOR SELECT * FROM current_check;
485+
486+
-- This tests the fetch-backwards code path
487+
FETCH ABSOLUTE 12 FROM c1;
488+
FETCH ABSOLUTE 8 FROM c1;
489+
DELETE FROM current_check WHERE CURRENT OF c1 RETURNING *;
490+
491+
-- This tests the ExecutorRewind code path
492+
FETCH ABSOLUTE 13 FROM c1;
493+
FETCH ABSOLUTE 1 FROM c1;
494+
DELETE FROM current_check WHERE CURRENT OF c1 RETURNING *;
495+
496+
SELECT * FROM current_check;
497+
ROLLBACK;
498+
475499
-- Make sure snapshot management works okay, per bug report in
476500
-- 235395b90909301035v7228ce63q392931f15aa74b31@mail.gmail.com
477501
BEGIN;

0 commit comments

Comments
 (0)