Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit00011a6

Browse files
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 agiven scan node, then rewind the cursor and fetch some row from anearlier scan node. In such a case, execCurrent.c mistakenly thoughtthat the later scan node was still active, because ExecReScan hadn'tdone anything to make it look not-active. We'd get some sort offailure in the case of a SeqScan node, because the node's scan tupleslot would be pointing at a HeapTuple whose t_self gets reset toinvalid by heapam.c. But it seems possible that for other relationscan node types we'd actually return a valid tuple TID to the caller,resulting in updating or deleting a tuple that shouldn't have beenconsidered current. To fix, forcibly clear the ScanTupleSlot inExecScanReScan.Another issue here, which seems only latent at the moment but couldeasily become a live bug in future, is that rewinding a cursor doesnot necessarily lead to *immediately* applying ExecReScan to everyscan-level node in the plan tree. Upper-level nodes will think thatthey can postpone that call if their child node is already markedwith chgParam flags. I don't see a way for that to happen today ina plan tree that's simple enough for execCurrent.c's search_plan_treeto understand, but that's one heck of a fragile assumption. So, addsome logic in search_plan_tree to detect chgParam flags being set onnodes that it descended to/through, and assume that that means weshould consider lower scan nodes to be logically reset even if theirReScan 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 parentdabdc7e commit00011a6

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
staticchar*fetch_cursor_param_value(ExprContext*econtext,intparamId);
26-
staticScanState*search_plan_tree(PlanState*node,Oidtable_oid);
26+
staticScanState*search_plan_tree(PlanState*node,Oidtable_oid,
27+
bool*pending_rescan);
2728

2829

2930
/*
@@ -156,8 +157,10 @@ execCurrentOf(CurrentOfExpr *cexpr,
156157
* aggregation.
157158
*/
158159
ScanState*scanstate;
160+
boolpending_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
/*
@@ -288,10 +295,20 @@ fetch_cursor_param_value(ExprContext *econtext, int paramId)
288295
*
289296
* Search through a PlanState tree for a scan node on the specified table.
290297
* Return NULL if not found or multiple candidates.
298+
*
299+
* If a candidate is found, set *pending_rescan to true if that candidate
300+
* or any node above it has a pending rescan action, i.e. chgParam != NULL.
301+
* That indicates that we shouldn't consider the node to be positioned on a
302+
* valid tuple, even if its own state would indicate that it is. (Caller
303+
* must initialize *pending_rescan to false, and should not trust its state
304+
* if multiple candidates are found.)
291305
*/
292306
staticScanState*
293-
search_plan_tree(PlanState*node,Oidtable_oid)
307+
search_plan_tree(PlanState*node,Oidtable_oid,
308+
bool*pending_rescan)
294309
{
310+
ScanState*result=NULL;
311+
295312
if (node==NULL)
296313
returnNULL;
297314
switch (nodeTag(node))
@@ -308,7 +325,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
308325
ScanState*sstate= (ScanState*)node;
309326

310327
if (RelationGetRelid(sstate->ss_currentRelation)==table_oid)
311-
returnsstate;
328+
result=sstate;
312329
break;
313330
}
314331

@@ -319,21 +336,21 @@ search_plan_tree(PlanState *node, Oid table_oid)
319336
caseT_AppendState:
320337
{
321338
AppendState*astate= (AppendState*)node;
322-
ScanState*result=NULL;
323339
inti;
324340

325341
for (i=0;i<astate->as_nplans;i++)
326342
{
327343
ScanState*elem=search_plan_tree(astate->appendplans[i],
328-
table_oid);
344+
table_oid,
345+
pending_rescan);
329346

330347
if (!elem)
331348
continue;
332349
if (result)
333350
returnNULL;/* multiple matches */
334351
result=elem;
335352
}
336-
returnresult;
353+
break;
337354
}
338355

339356
/*
@@ -342,21 +359,21 @@ search_plan_tree(PlanState *node, Oid table_oid)
342359
caseT_MergeAppendState:
343360
{
344361
MergeAppendState*mstate= (MergeAppendState*)node;
345-
ScanState*result=NULL;
346362
inti;
347363

348364
for (i=0;i<mstate->ms_nplans;i++)
349365
{
350366
ScanState*elem=search_plan_tree(mstate->mergeplans[i],
351-
table_oid);
367+
table_oid,
368+
pending_rescan);
352369

353370
if (!elem)
354371
continue;
355372
if (result)
356373
returnNULL;/* multiple matches */
357374
result=elem;
358375
}
359-
returnresult;
376+
break;
360377
}
361378

362379
/*
@@ -365,18 +382,31 @@ search_plan_tree(PlanState *node, Oid table_oid)
365382
*/
366383
caseT_ResultState:
367384
caseT_LimitState:
368-
returnsearch_plan_tree(node->lefttree,table_oid);
385+
result=search_plan_tree(node->lefttree,
386+
table_oid,
387+
pending_rescan);
388+
break;
369389

370390
/*
371391
* SubqueryScan too, but it keeps the child in a different place
372392
*/
373393
caseT_SubqueryScanState:
374-
returnsearch_plan_tree(((SubqueryScanState*)node)->subplan,
375-
table_oid);
394+
result=search_plan_tree(((SubqueryScanState*)node)->subplan,
395+
table_oid,
396+
pending_rescan);
397+
break;
376398

377399
default:
378400
/* Otherwise, assume we can't descend through it */
379401
break;
380402
}
381-
returnNULL;
403+
404+
/*
405+
* If we found a candidate at or below this node, then this node's
406+
* chgParam indicates a pending rescan that will affect the candidate.
407+
*/
408+
if (result&&node->chgParam!=NULL)
409+
*pending_rescan= true;
410+
411+
returnresult;
382412
}

‎src/backend/executor/execScan.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,12 @@ ExecScanReScan(ScanState *node)
337337
/* Stop projecting any tuples from SRFs in the targetlist */
338338
node->ps.ps_TupFromTlist= false;
339339

340+
/*
341+
* We must clear the scan tuple so that observers (e.g., execCurrent.c)
342+
* can tell that this plan node is not positioned on a tuple.
343+
*/
344+
ExecClearTuple(node->ss_ScanTupleSlot);
345+
340346
/* Rescan EvalPlanQual tuple if we're inside an EvalPlanQual recheck */
341347
if (estate->es_epqScanDone!=NULL)
342348
{

‎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 stringu1FROM onekWHERE stringu1='DZAAAA';
473473
ROLLBACK;
474474

475+
-- Check behavior with rewinding to a previous child scan node,
476+
-- as per bug #15395
477+
BEGIN;
478+
CREATETABLEcurrent_check (currentidint, payloadtext);
479+
CREATETABLEcurrent_check_1 () INHERITS (current_check);
480+
CREATETABLEcurrent_check_2 () INHERITS (current_check);
481+
INSERT INTO current_check_1SELECT i,'p'|| iFROM generate_series(1,9) i;
482+
INSERT INTO current_check_2SELECT i,'P'|| iFROM generate_series(10,19) i;
483+
484+
DECLARE c1 SCROLL CURSOR FORSELECT*FROM current_check;
485+
486+
-- This tests the fetch-backwards code path
487+
FETCH ABSOLUTE12FROM c1;
488+
FETCH ABSOLUTE8FROM c1;
489+
DELETEFROM current_checkWHERE CURRENT OF c1 RETURNING*;
490+
491+
-- This tests the ExecutorRewind code path
492+
FETCH ABSOLUTE13FROM c1;
493+
FETCH ABSOLUTE1FROM c1;
494+
DELETEFROM current_checkWHERE 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)
[8]ページ先頭
©2009-2025 Movatter.jp