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

Commitd8e877b

Browse files
committed
Repair more failures with SubPlans in multi-row VALUES lists.
Commit9b63c13 turns out to have been fundamentally misguided:the parent node's subPlan list is by no means the only way in whicha child SubPlan node can be hooked into the outer execution state.As shown in bug #16213 from Matt Jibson, we can also get short-livedtuple table slots added to the outer es_tupleTable list. At this pointI have little faith that there aren't other possible connections aswell; the long time it took to notice this problem shows that thisisn't a heavily-exercised situation.Therefore, revert that fix, returning to the coding that passed aNULL parent plan pointer down to the transiently-built subexpressions.That gives us a pretty good guarantee that they won't hook into theouter executor state in any way. But then we need some other solutionto make SubPlans work. Adopt the solution speculated about in theprevious commit's log message: do expression initialization at planstartup for just those VALUES rows containing SubPlans, abandoning thegoal of reclaiming memory intra-query for those rows. In practice itseems unlikely that queries containing a vast number of VALUES rowswould be using SubPlans in them, so this should not give up much.(BTW, this test case also refutes my claim in connection with the priorcommit that the issue only arises with use of LATERAL. That was justwrong: some variants of SubLink always produce SubPlans.)As with previous patch, back-patch to all supported branches.Discussion:https://postgr.es/m/16213-871ac3bc208ecf23@postgresql.org
1 parentfe955eb commitd8e877b

File tree

4 files changed

+119
-43
lines changed

4 files changed

+119
-43
lines changed

‎src/backend/executor/nodeValuesscan.c‎

Lines changed: 68 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include"executor/executor.h"
2727
#include"executor/nodeValuesscan.h"
2828
#include"jit/jit.h"
29+
#include"optimizer/clauses.h"
2930
#include"utils/expandeddatum.h"
3031

3132

@@ -50,7 +51,7 @@ ValuesNext(ValuesScanState *node)
5051
EState*estate;
5152
ExprContext*econtext;
5253
ScanDirectiondirection;
53-
List*exprlist;
54+
intcurr_idx;
5455

5556
/*
5657
* get information from the estate and scan state
@@ -67,19 +68,11 @@ ValuesNext(ValuesScanState *node)
6768
{
6869
if (node->curr_idx<node->array_len)
6970
node->curr_idx++;
70-
if (node->curr_idx<node->array_len)
71-
exprlist=node->exprlists[node->curr_idx];
72-
else
73-
exprlist=NIL;
7471
}
7572
else
7673
{
7774
if (node->curr_idx >=0)
7875
node->curr_idx--;
79-
if (node->curr_idx >=0)
80-
exprlist=node->exprlists[node->curr_idx];
81-
else
82-
exprlist=NIL;
8376
}
8477

8578
/*
@@ -90,16 +83,16 @@ ValuesNext(ValuesScanState *node)
9083
*/
9184
ExecClearTuple(slot);
9285

93-
if (exprlist)
86+
curr_idx=node->curr_idx;
87+
if (curr_idx >=0&&curr_idx<node->array_len)
9488
{
89+
List*exprlist=node->exprlists[curr_idx];
90+
List*exprstatelist=node->exprstatelists[curr_idx];
9591
MemoryContextoldContext;
96-
List*oldsubplans;
97-
List*exprstatelist;
9892
Datum*values;
9993
bool*isnull;
10094
ListCell*lc;
10195
intresind;
102-
intsaved_jit_flags;
10396

10497
/*
10598
* Get rid of any prior cycle's leftovers. We use ReScanExprContext
@@ -109,38 +102,32 @@ ValuesNext(ValuesScanState *node)
109102
ReScanExprContext(econtext);
110103

111104
/*
112-
* Build the expression eval state in the econtext's per-tuple memory.
113-
* This is a tad unusual, but we want to delete the eval state again
114-
* when we move to the next row, to avoid growth of memory
115-
* requirements over a long values list.
105+
* Do per-VALUES-row work in the per-tuple context.
116106
*/
117107
oldContext=MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
118108

119109
/*
120-
* The expressions might contain SubPlans (this is currently only
121-
* possible if there's a sub-select containing a LATERAL reference,
122-
* otherwise sub-selects in a VALUES list should be InitPlans). Those
123-
* subplans will want to hook themselves into our subPlan list, which
124-
* would result in a corrupted list after we delete the eval state. We
125-
* can work around this by saving and restoring the subPlan list.
126-
* (There's no need for the functionality that would be enabled by
127-
* having the list entries, since the SubPlans aren't going to be
128-
* re-executed anyway.)
129-
*/
130-
oldsubplans=node->ss.ps.subPlan;
131-
node->ss.ps.subPlan=NIL;
132-
133-
/*
134-
* As the expressions are only ever used once, disable JIT for them.
135-
* This is worthwhile because it's common to insert significant
136-
* amounts of data via VALUES().
110+
* Unless we already made the expression eval state for this row,
111+
* build it in the econtext's per-tuple memory. This is a tad
112+
* unusual, but we want to delete the eval state again when we move to
113+
* the next row, to avoid growth of memory requirements over a long
114+
* values list. For rows in which that won't work, we already built
115+
* the eval state at plan startup.
137116
*/
138-
saved_jit_flags=econtext->ecxt_estate->es_jit_flags;
139-
econtext->ecxt_estate->es_jit_flags=PGJIT_NONE;
140-
exprstatelist=ExecInitExprList(exprlist,&node->ss.ps);
141-
econtext->ecxt_estate->es_jit_flags=saved_jit_flags;
142-
143-
node->ss.ps.subPlan=oldsubplans;
117+
if (exprstatelist==NIL)
118+
{
119+
/*
120+
* Pass parent as NULL, not my plan node, because we don't want
121+
* anything in this transient state linking into permanent state.
122+
* The only expression type that might wish to do so is a SubPlan,
123+
* and we already checked that there aren't any.
124+
*
125+
* Note that passing parent = NULL also disables JIT compilation
126+
* of the expressions, which is a win, because they're only going
127+
* to be used once under normal circumstances.
128+
*/
129+
exprstatelist=ExecInitExprList(exprlist,NULL);
130+
}
144131

145132
/* parser should have checked all sublists are the same length */
146133
Assert(list_length(exprstatelist)==slot->tts_tupleDescriptor->natts);
@@ -281,13 +268,52 @@ ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
281268
scanstate->curr_idx=-1;
282269
scanstate->array_len=list_length(node->values_lists);
283270

284-
/* convert list of sublists into array of sublists for easy addressing */
271+
/*
272+
* Convert the list of expression sublists into an array for easier
273+
* addressing at runtime. Also, detect whether any sublists contain
274+
* SubPlans; for just those sublists, go ahead and do expression
275+
* initialization. (This avoids problems with SubPlans wanting to connect
276+
* themselves up to the outer plan tree. Notably, EXPLAIN won't see the
277+
* subplans otherwise; also we will have troubles with dangling pointers
278+
* and/or leaked resources if we try to handle SubPlans the same as
279+
* simpler expressions.)
280+
*/
285281
scanstate->exprlists= (List**)
286282
palloc(scanstate->array_len*sizeof(List*));
283+
scanstate->exprstatelists= (List**)
284+
palloc0(scanstate->array_len*sizeof(List*));
287285
i=0;
288286
foreach(vtl,node->values_lists)
289287
{
290-
scanstate->exprlists[i++]= (List*)lfirst(vtl);
288+
List*exprs=castNode(List,lfirst(vtl));
289+
290+
scanstate->exprlists[i]=exprs;
291+
292+
/*
293+
* We can avoid the cost of a contain_subplans() scan in the simple
294+
* case where there are no SubPlans anywhere.
295+
*/
296+
if (estate->es_subplanstates&&
297+
contain_subplans((Node*)exprs))
298+
{
299+
intsaved_jit_flags;
300+
301+
/*
302+
* As these expressions are only used once, disable JIT for them.
303+
* This is worthwhile because it's common to insert significant
304+
* amounts of data via VALUES(). Note that this doesn't prevent
305+
* use of JIT *within* a subplan, since that's initialized
306+
* separately; this just affects the upper-level subexpressions.
307+
*/
308+
saved_jit_flags=estate->es_jit_flags;
309+
estate->es_jit_flags=PGJIT_NONE;
310+
311+
scanstate->exprstatelists[i]=ExecInitExprList(exprs,
312+
&scanstate->ss.ps);
313+
314+
estate->es_jit_flags=saved_jit_flags;
315+
}
316+
i++;
291317
}
292318

293319
returnscanstate;

‎src/include/nodes/execnodes.h‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1540,14 +1540,21 @@ typedef struct FunctionScanState
15401540
*
15411541
*rowcontextper-expression-list context
15421542
*exprlistsarray of expression lists being evaluated
1543-
*array_lensize of array
1543+
*exprstatelistsarray of expression state lists, for SubPlans only
1544+
*array_lensize of above arrays
15441545
*curr_idxcurrent array index (0-based)
15451546
*
15461547
*Note: ss.ps.ps_ExprContext is used to evaluate any qual or projection
15471548
*expressions attached to the node. We create a second ExprContext,
15481549
*rowcontext, in which to build the executor expression state for each
15491550
*Values sublist. Resetting this context lets us get rid of expression
15501551
*state for each row, avoiding major memory leakage over a long values list.
1552+
*However, that doesn't work for sublists containing SubPlans, because a
1553+
*SubPlan has to be connected up to the outer plan tree to work properly.
1554+
*Therefore, for only those sublists containing SubPlans, we do expression
1555+
*state construction at executor start, and store those pointers in
1556+
*exprstatelists[]. NULL entries in that array correspond to simple
1557+
*subexpressions that are handled as described above.
15511558
* ----------------
15521559
*/
15531560
typedefstructValuesScanState
@@ -1557,6 +1564,8 @@ typedef struct ValuesScanState
15571564
List**exprlists;
15581565
intarray_len;
15591566
intcurr_idx;
1567+
/* in back branches, put this at the end to avoid ABI break: */
1568+
List**exprstatelists;
15601569
}ValuesScanState;
15611570

15621571
/* ----------------

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -894,6 +894,33 @@ where s.i < 10 and (select val.x) < 110;
894894
10
895895
(17 rows)
896896

897+
-- another variant of that (bug #16213)
898+
explain (verbose, costs off)
899+
select * from
900+
(values
901+
(3 not in (select * from (values (1), (2)) ss1)),
902+
(false)
903+
) ss;
904+
QUERY PLAN
905+
----------------------------------------
906+
Values Scan on "*VALUES*"
907+
Output: "*VALUES*".column1
908+
SubPlan 1
909+
-> Values Scan on "*VALUES*_1"
910+
Output: "*VALUES*_1".column1
911+
(5 rows)
912+
913+
select * from
914+
(values
915+
(3 not in (select * from (values (1), (2)) ss1)),
916+
(false)
917+
) ss;
918+
column1
919+
---------
920+
t
921+
f
922+
(2 rows)
923+
897924
--
898925
-- Check sane behavior with nested IN SubLinks
899926
--

‎src/test/regress/sql/subselect.sql‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,20 @@ select val.x
492492
)as val(x)
493493
wheres.i<10and (selectval.x)<110;
494494

495+
-- another variant of that (bug #16213)
496+
explain (verbose, costs off)
497+
select*from
498+
(values
499+
(3 notin (select*from (values (1), (2)) ss1)),
500+
(false)
501+
) ss;
502+
503+
select*from
504+
(values
505+
(3 notin (select*from (values (1), (2)) ss1)),
506+
(false)
507+
) ss;
508+
495509
--
496510
-- Check sane behavior with nested IN SubLinks
497511
--

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp