pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #16213: segfault when running a query
3+ messages / 2 participants
[nested] [flat]
* BUG #16213: segfault when running a query
@ 2020-01-16 23:27 PG Bug reporting form <noreply@postgresql.org>
2020-01-17 04:03 ` Re: BUG #16213: segfault when running a query Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 3+ messages in thread
From: PG Bug reporting form @ 2020-01-16 23:27 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: matt.jibson@gmail.com
The following bug has been logged on the website:
Bug reference: 16213
Logged by: Matt Jibson
Email address: matt.jibson@gmail.com
PostgreSQL version: 11.5
Operating system: linux
Description:
SELECT
*
FROM
(
SELECT
tab_31924.col_41292 AS col_41294,
tab_31924.col_41293 AS col_41295,
0::OID AS col_41296,
false AS col_41297
FROM
(
VALUES
(
'A'::STRING::STRING
NOT IN (
SELECT
'E'::STRING::STRING AS col_41289
FROM
(
VALUES
(NULL),
(NULL),
(NULL),
(NULL)
)
AS tab_31923 (col_41288)
WHERE
false
),
NULL,
'B'::STRING,
3::OID
),
(false, 4::OID, 'B'::STRING, 0::OID)
)
AS tab_31924
(col_41290, col_41291, col_41292, col_41293)
WHERE
tab_31924.col_41290
)
AS tab_31925
ORDER BY
col_41294 NULLS FIRST,
col_41295 NULLS FIRST,
col_41296 NULLS FIRST,
col_41297 NULLS FIRST;
The above query produces an error in the server log:
LOG: server process (PID 108) was terminated by signal 11: Segmentation
fault
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG #16213: segfault when running a query
2020-01-16 23:27 BUG #16213: segfault when running a query PG Bug reporting form <noreply@postgresql.org>
@ 2020-01-17 04:03 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-01-17 18:29 ` Re: BUG #16213: segfault when running a query Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2020-01-17 04:03 UTC (permalink / raw)
To: matt.jibson@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> writes:
> The above query produces an error in the server log:
> LOG: server process (PID 108) was terminated by signal 11: Segmentation
> fault
Yeah, duplicated here. (For anyone following along at home: you
can "create domain string as text", or just s/STRING/TEXT/g in the
given query. That type's not relevant to the problem.) The problem
is probably much more easily reproduced in a debug build, because it
boils down to a dangling-pointer bug. I duplicated it back to 9.4,
and it's probably older than that.
The direct cause of the crash is that by the time we get to ExecutorEnd,
there are dangling pointers in the es_tupleTable list. Those pointers
turn out to originate from ExecInitSubPlan's creation of TupleTableSlots
for the ProjectionInfo objects it creates when doing hashing. And the
reason they're dangling is that the subplan is inside a VALUES list,
and nodeValuesscan.c does this remarkably bletcherous thing:
* Build the expression eval state in the econtext's per-tuple memory.
* This is a tad unusual, but we want to delete the eval state again
* when we move to the next row, to avoid growth of memory
* requirements over a long values list.
It turns out that just below that, there's already some messy hacking
to deal with subplans in the VALUES list. But I guess we'd not hit
the case of a subplan using hashing within VALUES.
The attached draft patch fixes this by not letting ExecInitSubPlan hook
the slots it's making into the outer es_tupleTable list. Ordinarily
that would be bad because it exposes us to leaking resources, if the
slots aren't cleared before ending execution. But nodeSubplan.c is
already being (mostly) careful to clear those slots promptly, so it
doesn't cost us anything much to lose this backstop.
What that change fails to do is guarantee that there are no such
bugs elsewhere. In the attached I made nodeValuesscan.c assert that
nothing has added slots to the es_tupleTable list, but of course
that only catches cases where there's a live bug. Given how long
this case escaped detection, I don't feel like that's quite enough.
(Unsurprisingly, the existing regression tests don't trigger this
assert, even without the nodeSubplan.c fix.)
Another angle I've not run to ground is that the comments for the
existing subplan-related hacking in nodeValuesscan.c claim that
a problematic subplan could only occur in conjunction with LATERAL.
But there's no LATERAL in this example --- are those comments wrong
or obsolete, or just talking about a different case?
I didn't work on making a minimal test case for the regression tests,
either.
Anyway, thanks for the report!
regards, tom lane
Attachments:
[text/x-diff] fix-for-hashed-subplan-in-VALUES-1.patch (5.4K, ../../635.1579233826@sss.pgh.pa.us/2-fix-for-hashed-subplan-in-VALUES-1.patch)
download | inline diff:
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index ff95317..8a4084b 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -133,11 +133,15 @@ ExecHashSubPlan(SubPlanState *node,
slot = ExecProject(node->projLeft);
/*
- * Note: because we are typically called in a per-tuple context, we have
- * to explicitly clear the projected tuple before returning. Otherwise,
- * we'll have a double-free situation: the per-tuple context will probably
- * be reset before we're called again, and then the tuple slot will think
- * it still needs to free the tuple.
+ * Note: we have to explicitly clear the projected tuple before returning.
+ * This guards against two different hazards. First, because we are
+ * typically called in a per-tuple context, the data that was stored into
+ * the slot may be shorter-lived than the slot. If that data is cleared
+ * before we're called again, but the tuple slot still thinks it needs to
+ * free the tuple, we'll have a double-free crash. Second, because the
+ * slot isn't hooked into any estate tuple-table list, there isn't anyone
+ * else who's going to clear it before executor shutdown. So we risk
+ * leaking buffer pins or the like if we don't clear it here.
*/
/*
@@ -605,21 +609,17 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
}
/*
+ * As with projLeft, we must manually clear the slot after each use.
+ */
+ ExecClearTuple(slot);
+
+ /*
* Reset innerecontext after each inner tuple to free any memory used
* during ExecProject.
*/
ResetExprContext(innerecontext);
}
- /*
- * Since the projected tuples are in the sub-query's context and not the
- * main context, we'd better clear the tuple slot before there's any
- * chance of a reset of the sub-query's context. Else we will have the
- * potential for a double free attempt. (XXX possibly no longer needed,
- * but can't hurt.)
- */
- ExecClearTuple(node->projRight->pi_state.resultslot);
-
MemoryContextSwitchTo(oldcontext);
}
@@ -989,9 +989,15 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
* Fortunately we can just pass NULL for now and fill it in later
* (hack alert!). The righthand expressions will be evaluated in our
* own innerecontext.
+ *
+ * We intentionally don't add the slots to the estate's es_tupleTable.
+ * That's to support execution of subplans within short-lived
+ * contexts: these slots might get freed before the estate ends
+ * execution. Because of this, we have to be careful to explicitly
+ * clear each slot as soon as we're done with its current value.
*/
tupDescLeft = ExecTypeFromTL(lefttlist);
- slot = ExecInitExtraTupleSlot(estate, tupDescLeft, &TTSOpsVirtual);
+ slot = MakeTupleTableSlot(tupDescLeft, &TTSOpsVirtual);
sstate->projLeft = ExecBuildProjectionInfo(lefttlist,
NULL,
slot,
@@ -999,7 +1005,7 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
NULL);
sstate->descRight = tupDescRight = ExecTypeFromTL(righttlist);
- slot = ExecInitExtraTupleSlot(estate, tupDescRight, &TTSOpsVirtual);
+ slot = MakeTupleTableSlot(tupDescRight, &TTSOpsVirtual);
sstate->projRight = ExecBuildProjectionInfo(righttlist,
sstate->innerecontext,
slot,
diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 8d916a0..f104d8c 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -94,6 +94,7 @@ ValuesNext(ValuesScanState *node)
{
MemoryContext oldContext;
List *oldsubplans;
+ List *oldtupletable;
List *exprstatelist;
Datum *values;
bool *isnull;
@@ -131,15 +132,38 @@ ValuesNext(ValuesScanState *node)
node->ss.ps.subPlan = NIL;
/*
+ * A related problem is that SubPlans might try to create tuple slots
+ * and attach them to the outer estate's es_tupleTable; that would
+ * result in dangling pointers in the es_tupleTable after we delete
+ * the eval state. We can prevent the dangling-pointer problem by
+ * similarly saving and restoring the es_tupleTable list. This is
+ * only a partial solution though: any slots thereby excluded from the
+ * outer estate might result in resource leaks, if they're not cleared
+ * before ending execution. Therefore, we decree that SubPlans
+ * mustn't add slots to the outer es_tupleTable. The Assert below
+ * checks that. It's only an Assert because resource leakage is way
+ * less problematic than a dangling pointer.
+ */
+ oldtupletable = econtext->ecxt_estate->es_tupleTable;
+ econtext->ecxt_estate->es_tupleTable = NIL;
+
+ /*
* As the expressions are only ever used once, disable JIT for them.
* This is worthwhile because it's common to insert significant
* amounts of data via VALUES().
*/
saved_jit_flags = econtext->ecxt_estate->es_jit_flags;
econtext->ecxt_estate->es_jit_flags = PGJIT_NONE;
+
+ /* OK, finally we can init the expressions */
exprstatelist = ExecInitExprList(exprlist, &node->ss.ps);
+
+ /* Undo the hacks above */
econtext->ecxt_estate->es_jit_flags = saved_jit_flags;
+ Assert(econtext->ecxt_estate->es_tupleTable == NIL);
+ econtext->ecxt_estate->es_tupleTable = oldtupletable;
+
node->ss.ps.subPlan = oldsubplans;
/* parser should have checked all sublists are the same length */
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG #16213: segfault when running a query
2020-01-16 23:27 BUG #16213: segfault when running a query PG Bug reporting form <noreply@postgresql.org>
2020-01-17 04:03 ` Re: BUG #16213: segfault when running a query Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-01-17 18:29 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 3+ messages in thread
From: Tom Lane @ 2020-01-17 18:29 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: matt.jibson@gmail.com; pgsql-bugs@lists.postgresql.org
I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> The above query produces an error in the server log:
>> LOG: server process (PID 108) was terminated by signal 11: Segmentation
>> fault
> The direct cause of the crash is that by the time we get to ExecutorEnd,
> there are dangling pointers in the es_tupleTable list.
After further reflection, I'm totally dissatisfied with the quick-hack
patch I posted last night. I think that what this example demonstrates
is that my fix (in commit 9b63c13f0) for bug #14924 in fact does not
work: the subPlan list is not the only way in which a SubPlan connects
up to the outer plan level. I have no faith that the es_tupleTable list
is the only other way, either, or that we won't create more in future.
I think what we have to do here is revert 9b63c13f0, going back to
the previous policy of passing down parent = NULL to the transient
subexpressions, so that there is a strong guarantee that there aren't
any unwanted connections between short-lived and longer-lived state.
And then we need some other solution for making SubPlans in VALUES
lists work. The best bet seems to be what I speculated about in that
commit message: initialize the expressions for VALUES rows that contain
SubPlans normally at executor startup, and use the trick with
short-lived expression state only for VALUES rows that don't contain any
SubPlans. I think that the case we're worried about with long VALUES
lists is not likely to involve any SubPlans, so that this shouldn't be
too awful for memory consumption.
Another benefit of doing it like this is that SubPlans in the VALUES
lists are reported normally by EXPLAIN, while the previous hack caused
them to be missing from the output.
Objections, better ideas?
regards, tom lane
Attachments:
[text/x-diff] fix-for-hashed-subplan-in-VALUES-2.patch (9.2K, ../../31142.1579285794@sss.pgh.pa.us/2-fix-for-hashed-subplan-in-VALUES-2.patch)
download | inline diff:
diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 8d916a0..8866121 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -26,6 +26,7 @@
#include "executor/executor.h"
#include "executor/nodeValuesscan.h"
#include "jit/jit.h"
+#include "optimizer/clauses.h"
#include "utils/expandeddatum.h"
@@ -50,7 +51,7 @@ ValuesNext(ValuesScanState *node)
EState *estate;
ExprContext *econtext;
ScanDirection direction;
- List *exprlist;
+ int curr_idx;
/*
* get information from the estate and scan state
@@ -67,19 +68,11 @@ ValuesNext(ValuesScanState *node)
{
if (node->curr_idx < node->array_len)
node->curr_idx++;
- if (node->curr_idx < node->array_len)
- exprlist = node->exprlists[node->curr_idx];
- else
- exprlist = NIL;
}
else
{
if (node->curr_idx >= 0)
node->curr_idx--;
- if (node->curr_idx >= 0)
- exprlist = node->exprlists[node->curr_idx];
- else
- exprlist = NIL;
}
/*
@@ -90,16 +83,16 @@ ValuesNext(ValuesScanState *node)
*/
ExecClearTuple(slot);
- if (exprlist)
+ curr_idx = node->curr_idx;
+ if (curr_idx >= 0 && curr_idx < node->array_len)
{
+ List *exprlist = node->exprlists[curr_idx];
+ List *exprstatelist = node->exprstatelists[curr_idx];
MemoryContext oldContext;
- List *oldsubplans;
- List *exprstatelist;
Datum *values;
bool *isnull;
ListCell *lc;
int resind;
- int saved_jit_flags;
/*
* Get rid of any prior cycle's leftovers. We use ReScanExprContext
@@ -109,38 +102,32 @@ ValuesNext(ValuesScanState *node)
ReScanExprContext(econtext);
/*
- * Build the expression eval state in the econtext's per-tuple memory.
- * This is a tad unusual, but we want to delete the eval state again
- * when we move to the next row, to avoid growth of memory
- * requirements over a long values list.
+ * Do per-VALUES-row work in the per-tuple context.
*/
oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
/*
- * The expressions might contain SubPlans (this is currently only
- * possible if there's a sub-select containing a LATERAL reference,
- * otherwise sub-selects in a VALUES list should be InitPlans). Those
- * subplans will want to hook themselves into our subPlan list, which
- * would result in a corrupted list after we delete the eval state. We
- * can work around this by saving and restoring the subPlan list.
- * (There's no need for the functionality that would be enabled by
- * having the list entries, since the SubPlans aren't going to be
- * re-executed anyway.)
- */
- oldsubplans = node->ss.ps.subPlan;
- node->ss.ps.subPlan = NIL;
-
- /*
- * As the expressions are only ever used once, disable JIT for them.
- * This is worthwhile because it's common to insert significant
- * amounts of data via VALUES().
+ * Unless we already made the expression eval state for this row,
+ * build it in the econtext's per-tuple memory. This is a tad
+ * unusual, but we want to delete the eval state again when we move to
+ * the next row, to avoid growth of memory requirements over a long
+ * values list. For rows in which that won't work, we already built
+ * the eval state at plan startup.
*/
- saved_jit_flags = econtext->ecxt_estate->es_jit_flags;
- econtext->ecxt_estate->es_jit_flags = PGJIT_NONE;
- exprstatelist = ExecInitExprList(exprlist, &node->ss.ps);
- econtext->ecxt_estate->es_jit_flags = saved_jit_flags;
-
- node->ss.ps.subPlan = oldsubplans;
+ if (exprstatelist == NIL)
+ {
+ /*
+ * Pass parent as NULL, not my plan node, because we don't want
+ * anything in this transient state linking into permanent state.
+ * The only expression type that might wish to do so is a SubPlan,
+ * and we already checked that there aren't any.
+ *
+ * Note that passing parent = NULL also disables JIT compilation
+ * of the expressions, which is a win, because they're only going
+ * to be used once under normal circumstances.
+ */
+ exprstatelist = ExecInitExprList(exprlist, NULL);
+ }
/* parser should have checked all sublists are the same length */
Assert(list_length(exprstatelist) == slot->tts_tupleDescriptor->natts);
@@ -281,13 +268,52 @@ ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
scanstate->curr_idx = -1;
scanstate->array_len = list_length(node->values_lists);
- /* convert list of sublists into array of sublists for easy addressing */
+ /*
+ * Convert the list of expression sublists into an array for easier
+ * addressing at runtime. Also, detect whether any sublists contain
+ * SubPlans; for just those sublists, go ahead and do expression
+ * initialization. (This avoids problems with SubPlans wanting to connect
+ * themselves up to the outer plan tree. Notably, EXPLAIN won't see the
+ * subplans otherwise; also we will have troubles with dangling pointers
+ * and/or leaked resources if we try to handle SubPlans the same as
+ * simpler expressions.)
+ */
scanstate->exprlists = (List **)
palloc(scanstate->array_len * sizeof(List *));
+ scanstate->exprstatelists = (List **)
+ palloc0(scanstate->array_len * sizeof(List *));
i = 0;
foreach(vtl, node->values_lists)
{
- scanstate->exprlists[i++] = (List *) lfirst(vtl);
+ List *exprs = castNode(List, lfirst(vtl));
+
+ scanstate->exprlists[i] = exprs;
+
+ /*
+ * We can avoid the cost of a contain_subplans() scan in the simple
+ * case where there are no SubPlans anywhere.
+ */
+ if (estate->es_subplanstates &&
+ contain_subplans((Node *) exprs))
+ {
+ int saved_jit_flags;
+
+ /*
+ * As these expressions are only used once, disable JIT for them.
+ * This is worthwhile because it's common to insert significant
+ * amounts of data via VALUES(). Note that this doesn't prevent
+ * use of JIT *within* a subplan, since that's initialized
+ * separately; this just affects the upper-level subexpressions.
+ */
+ saved_jit_flags = estate->es_jit_flags;
+ estate->es_jit_flags = PGJIT_NONE;
+
+ scanstate->exprstatelists[i] = ExecInitExprList(exprs,
+ &scanstate->ss.ps);
+
+ estate->es_jit_flags = saved_jit_flags;
+ }
+ i++;
}
return scanstate;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index eaea1f3..1f6f5bb 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1670,7 +1670,8 @@ typedef struct FunctionScanState
*
* rowcontext per-expression-list context
* exprlists array of expression lists being evaluated
- * array_len size of array
+ * exprstatelists array of expression state lists, for SubPlans only
+ * array_len size of above arrays
* curr_idx current array index (0-based)
*
* Note: ss.ps.ps_ExprContext is used to evaluate any qual or projection
@@ -1678,6 +1679,12 @@ typedef struct FunctionScanState
* rowcontext, in which to build the executor expression state for each
* Values sublist. Resetting this context lets us get rid of expression
* state for each row, avoiding major memory leakage over a long values list.
+ * However, that doesn't work for sublists containing SubPlans, because a
+ * SubPlan has to be connected up to the outer plan tree to work properly.
+ * Therefore, for only those sublists containing SubPlans, we do expression
+ * state construction at executor start, and store those pointers in
+ * exprstatelists[]. NULL entries in that array correspond to simple
+ * subexpressions that are handled as described above.
* ----------------
*/
typedef struct ValuesScanState
@@ -1685,6 +1692,7 @@ typedef struct ValuesScanState
ScanState ss; /* its first field is NodeTag */
ExprContext *rowcontext;
List **exprlists;
+ List **exprstatelists;
int array_len;
int curr_idx;
} ValuesScanState;
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 5476861..71a677b 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -926,6 +926,33 @@ where s.i < 10 and (select val.x) < 110;
10
(17 rows)
+-- another variant of that (bug #16213)
+explain (verbose, costs off)
+select * from
+(values
+ (3 not in (select * from (values (1), (2)) ss1)),
+ (false)
+) ss;
+ QUERY PLAN
+----------------------------------------
+ Values Scan on "*VALUES*"
+ Output: "*VALUES*".column1
+ SubPlan 1
+ -> Values Scan on "*VALUES*_1"
+ Output: "*VALUES*_1".column1
+(5 rows)
+
+select * from
+(values
+ (3 not in (select * from (values (1), (2)) ss1)),
+ (false)
+) ss;
+ column1
+---------
+ t
+ f
+(2 rows)
+
--
-- Check sane behavior with nested IN SubLinks
--
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 3e119ce..bd8d2f6 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -518,6 +518,20 @@ select val.x
) as val(x)
where s.i < 10 and (select val.x) < 110;
+-- another variant of that (bug #16213)
+explain (verbose, costs off)
+select * from
+(values
+ (3 not in (select * from (values (1), (2)) ss1)),
+ (false)
+) ss;
+
+select * from
+(values
+ (3 not in (select * from (values (1), (2)) ss1)),
+ (false)
+) ss;
+
--
-- Check sane behavior with nested IN SubLinks
--
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2020-01-17 18:29 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 23:27 BUG #16213: segfault when running a query PG Bug reporting form <noreply@postgresql.org>
2020-01-17 04:03 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-01-17 18:29 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox