Обсуждение: Consistent segfault in complex query
Hello,
We encountered a query that has been able to frequently segfault one of our postgres instances under certain conditions
whichwe have not fully been able to isolate for reproduction. We were able to get a core dump out of one of the crashes
andhave poked at it, but we believe the answer is beyond our knowledge of postgres internals. This is on a 9.3.19
serverand we saw no mention of a fix in the release notes since this version and we do not know if it affects later
majorreleases as well.
What we know so far is that somehow the arraycontains function was given a datum of 0 as the second argument that
dereferencedto a null pointer. Our current hypothesis from poking at the core dump is that some memory context is
gettingfreed before it should. This assumption comes from the complexity in the query (CTE containing params being
repeatedlyevaluated by multiple case statements) and the unpredictability of the failure case.
The issue is easily avoidable, and we have asked the developer to solve their problem differently. However, the
existenceof a segfault is always concerning and we are reporting this issue in an effort to be conscientiousness
membersof the community.
Due to the potentially sensitive contents we cannot provide the core directly, but we are happy to run commands against
thecore file to extract debugging information. We have also replaced certain values (database name, table name, column
name)with generic identifiers.
version
----------------------------------------------------------------------------------------------------------------
PostgreSQL 9.3.19 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-18), 64-bit
(1 row)
Query:
WITH tmp(foo2, bar2, baz2, minutes2) AS
(
SELECT ARRAY[$1 ::text], ARRAY[$2 ::text], ARRAY[$3 ::text], $4 ::double precision
)
UPDATE table_name
SET occurrences = CASE
WHEN time_last_noticed >= current_timestamp - (SELECT minutes2 FROM tmp) * interval '1 minutes'
THEN occurrences + 1
ELSE 1
END,
foo = CASE
WHEN time_last_noticed < current_timestamp - (SELECT minutes2 FROM tmp) * interval '1 minutes'
THEN (SELECT foo2 FROM tmp)
WHEN foo @> (SELECT foo2 FROM tmp)
THEN foo
ELSE array_cat(foo, (SELECT foo2 FROM tmp))
END,
bar = CASE
WHEN time_last_noticed < current_timestamp - (SELECT minutes2 FROM tmp) * interval '1 minutes'
THEN (SELECT bar2 FROM tmp)
WHEN bar @> (SELECT bar2 FROM tmp)
THEN bar
ELSE array_cat(bar, (SELECT bar2 FROM tmp))
END,
baz = CASE
WHEN time_last_noticed < current_timestamp - (SELECT minutes2 FROM tmp) * interval '1 minutes'
THEN (SELECT baz2 FROM tmp)
WHEN baz @> (SELECT baz2 FROM tmp)
THEN baz
ELSE array_cat(baz, (SELECT baz2 FROM tmp))
END,
time_last_noticed = current_timestamp,
total_occurrences = total_occurrences + 1
WHERE id1 = $5 AND id2 = $6;
Table schema:
Table "public.table_name"
Column | Type | Modifiers | Storage | Stats target | Description
-------------------+-----------------------------+-----------+----------+--------------+-------------
id1 | text | not null | extended | |
id2 | text | not null | extended | |
occurrences | integer | not null | plain | |
time_last_noticed | timestamp without time zone | not null | plain | |
total_occurrences | integer | not null | plain | |
bar | text[] | | extended | |
baz | text[] | | extended | |
foo | text[] | | extended | |
Indexes:
"table_name_pkey" PRIMARY KEY, btree (id1, id2)
Back trace from the dump:
(gdb) bt
#0 pg_detoast_datum (datum=0x0) at fmgr.c:2241
#1 0x000000000067cd90 in arraycontains (fcinfo=0x2c56e30) at arrayfuncs.c:3841
#2 0x000000000058ba25 in ExecMakeFunctionResultNoSets (fcache=0x2c56dc0, econtext=0x2c500c0, isNull=0x7ffc571d9a3f
"",isDone=<value optimized out>) at execQual.c:2027
#3 0x0000000000587375 in ExecEvalCase (caseExpr=0x2c54a30, econtext=0x2c500c0, isNull=0x2c60967 "",
isDone=0x2c60c4c)at execQual.c:2985
#4 0x00000000005878c3 in ExecTargetList (projInfo=<value optimized out>, isDone=0x7ffc571d9b0c) at execQual.c:5322
#5 ExecProject (projInfo=<value optimized out>, isDone=0x7ffc571d9b0c) at execQual.c:5537
#6 0x000000000058dd62 in ExecScan (node=0x2c4ffb0, accessMtd=0x599340 <IndexNext>, recheckMtd=0x5992f0
<IndexRecheck>)at execScan.c:207
#7 0x0000000000586e28 in ExecProcNode (node=0x2c4ffb0) at execProcnode.c:404
#8 0x0000000000584472 in EvalPlanQualNext (epqstate=<value optimized out>) at execMain.c:2366
#9 0x0000000000584b47 in EvalPlanQual (estate=0x2bdd240, epqstate=0x2c0d638, relation=<value optimized out>,
rti=1,lockmode=<value optimized out>, tid=0x7ffc571d9c60, priorXmax=2185728490) at execMain.c:1951
#10 0x000000000059d7cb in ExecUpdate (node=0x2c0d598) at nodeModifyTable.c:727
#11 ExecModifyTable (node=0x2c0d598) at nodeModifyTable.c:997
#12 0x0000000000586e78 in ExecProcNode (node=0x2c0d598) at execProcnode.c:377
#13 0x0000000000585a82 in ExecutePlan (queryDesc=0x2bdd000, direction=<value optimized out>, count=0) at
execMain.c:1488
#14 standard_ExecutorRun (queryDesc=0x2bdd000, direction=<value optimized out>, count=0) at execMain.c:318
#15 0x00007f2e5f3b358b in explain_ExecutorRun (queryDesc=0x2bdd000, direction=ForwardScanDirection, count=0) at
auto_explain.c:231
#16 0x00007f2e5f1af495 in pgss_ExecutorRun (queryDesc=0x2bdd000, direction=ForwardScanDirection, count=0) at
pg_stat_statements.c:716
#17 0x000000000066553f in ProcessQuery (plan=0x2be32b0, sourceText=0x2be7fe0 "WITH tmp(foo2, bar2, baz2, minutes2)
AS( SELECT ARRAY[$1 ::text], ARRAY[$2 ::text], ARRAY[$3 ::text], $4 ::double precision ) UPDATE table_name SET
occurrences= CAS"..., params=0x2bdce30, dest=<value optimized out>, completionTag=0x7ffc571da2f0 "") at pquery.c:185
#18 0x000000000066576f in PortalRunMulti (portal=0x2bd2530, isTopLevel=1 '\001', dest=0xb1d380, altdest=0xb1d380,
completionTag=0x7ffc571da2f0"") at pquery.c:1275
#19 0x0000000000665e32 in PortalRun (portal=0x2bd2530, count=9223372036854775807, isTopLevel=1 '\001',
dest=0x2ba30b0,altdest=0x2ba30b0, completionTag=0x7ffc571da2f0 "") at pquery.c:812
#20 0x000000000066409d in exec_execute_message (argc=<value optimized out>, argv=<value optimized out>,
dbname=0x2965280"database_name", username=<value optimized out>) at postgres.c:1958
#21 PostgresMain (argc=<value optimized out>, argv=<value optimized out>, dbname=0x2965280 "database_name",
username=<valueoptimized out>) at postgres.c:4154
#22 0x000000000061df38 in BackendRun (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:4194
#23 BackendStartup (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:3857
#24 ServerLoop (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:1702
#25 PostmasterMain (argc=<value optimized out>, argv=<value optimized out>) at postmaster.c:1326
#26 0x00000000005b92f8 in main (argc=5, argv=0x2964470) at main.c:234
fcinfo contents from the arraycontains in frame #1
(gdb) print *fcinfo
$1 = {flinfo = 0x2c56de0, context = 0x0, resultinfo = 0x0, fncollation = 100, isnull = 0 '\000', nargs = 2, arg =
{46431788,0 <repeats 99 times>}, argnull = '\000' <repeats 99 times>}
- Kyle Samson
>>>>> "Kyle" == Kyle Samson <kysamson@tripadvisor.com> writes:
Kyle> Hello,
Kyle> We encountered a query that has been able to frequently segfault
Kyle> one of our postgres instances under certain conditions which we
Kyle> have not fully been able to isolate for reproduction. We were
Kyle> able to get a core dump out of one of the crashes and have poked
Kyle> at it, but we believe the answer is beyond our knowledge of
Kyle> postgres internals. This is on a 9.3.19 server and we saw no
Kyle> mention of a fix in the release notes since this version and we
Kyle> do not know if it affects later major releases as well.
There's a relevant commit from Feb this year (ea6d67cf8) specifically
referring to the case of CTEs inside subplans inside EvalPlanQual, which
is exactly the scenario you have in your query. So you need to try this
in 9.3.22 or later (ideally 9.3.24, the latest) which contain this fix.
This is the relevant release note:
* Fix misbehavior of concurrent-update rechecks with CTE references
appearing in subplans (Tom Lane)
If a CTE (WITH clause reference) is used in an InitPlan or SubPlan,
and the query requires a recheck due to trying to update or lock a
concurrently-updated row, incorrect results could be obtained.
If this is indeed the problem, you may be able to narrow down the
required conditions more tightly: the problem will occur only if the row
to be updated was concurrently updated by another transaction. This
shouldn't be too hard to arrange - update a row in another transaction
but don't commit it yet, run the failing update statement such that it
will update that same row (it will block), then commit the first update.
--
Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Kyle" == Kyle Samson <kysamson@tripadvisor.com> writes:
> Kyle> We encountered a query that has been able to frequently segfault
> Kyle> one of our postgres instances under certain conditions which we
> Kyle> have not fully been able to isolate for reproduction. We were
> Kyle> able to get a core dump out of one of the crashes and have poked
> Kyle> at it, but we believe the answer is beyond our knowledge of
> Kyle> postgres internals. This is on a 9.3.19 server and we saw no
> Kyle> mention of a fix in the release notes since this version and we
> Kyle> do not know if it affects later major releases as well.
> There's a relevant commit from Feb this year (ea6d67cf8) specifically
> referring to the case of CTEs inside subplans inside EvalPlanQual, which
> is exactly the scenario you have in your query. So you need to try this
> in 9.3.22 or later (ideally 9.3.24, the latest) which contain this fix.
I'm not entirely convinced that that fix will cure this, but certainly
it seems related, and we should find out whether it has any effect.
The reason this seems possibly different is that we're apparently
returning wrong data out of the sub-select (a zero Datum value, but
not marked isnull --- if it were, arraycontains wouldn't be reached).
The previously fixed bug would have caused either multiple or missed
returns of a valid CTE tuple.
> If this is indeed the problem, you may be able to narrow down the
> required conditions more tightly: the problem will occur only if the row
> to be updated was concurrently updated by another transaction.
Yeah, the presence of EvalPlanQual in the backtrace is sufficient
to confirm that. It should be pretty easy to make a reproducible
test case once you understand that prerequisite.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> There's a relevant commit from Feb this year (ea6d67cf8) >> specifically referring to the case of CTEs inside subplans inside >> EvalPlanQual, which is exactly the scenario you have in your query. >> So you need to try this in 9.3.22 or later (ideally 9.3.24, the >> latest) which contain this fix. Tom> I'm not entirely convinced that that fix will cure this, but Tom> certainly it seems related, and we should find out whether it has Tom> any effect. I agree. Tom> The reason this seems possibly different is that we're apparently Tom> returning wrong data out of the sub-select (a zero Datum value, Tom> but not marked isnull --- if it were, arraycontains wouldn't be Tom> reached). The previously fixed bug would have caused either Tom> multiple or missed returns of a valid CTE tuple. I have some ideas as to why, and I'm poking at them in order to create a test case (no luck yet, but I'll keep at it). -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Tom> The reason this seems possibly different is that we're apparently Tom> returning wrong data out of the sub-select (a zero Datum value, Tom> but not marked isnull --- if it were, arraycontains wouldn't be Tom> reached). The previously fixed bug would have caused either Tom> multiple or missed returns of a valid CTE tuple. Andrew> I have some ideas as to why, and I'm poking at them in order to Andrew> create a test case (no luck yet, but I'll keep at it). Bingo - I have a test case, which I'll post in a sec after testing it on other versions. The key in this case is that the EPQ is the _first_ time the InitPlan is executed - you need a construct like this: case when flag then foo when foo @> (select ... from cte) then foo end such that flag is true on the initially visible row version (hence the initplan is not run yet), but false on the modified version (hence running the initplan during EPQ). -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >>>>> "Kyle" == Kyle Samson <kysamson@tripadvisor.com> writes: Kyle> This is on a 9.3.19 server and we saw no Kyle> mention of a fix in the release notes since this version and we Kyle> do not know if it affects later major releases as well. Andrew> There's a relevant commit from Feb this year (ea6d67cf8) Andrew> specifically referring to the case of CTEs inside subplans Andrew> inside EvalPlanQual, which is exactly the scenario you have in Andrew> your query. So you need to try this in 9.3.22 or later (ideally Andrew> 9.3.24, the latest) which contain this fix. Kyle, you can disregard this suggestion because I've now confirmed the bug still exists in 9.3.24 (actually in REL9_3_STABLE head). -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Andrew> Bingo - I have a test case, which I'll post in a sec after
Andrew> testing it on other versions.
OK, not only does it break in latest 9.3 stable, it also breaks in
current master.
This is the testcase:
create table mytable (id integer, foo text[] default '{}', flag boolean default false);
insert into mytable select generate_series(1,10);
now in session B do:
begin; update mytable set foo='{baz}', flag=true where id=6;
-- leave transaction open
and in session A:
with tmp(f2) as (select array['foo'])
update mytable set foo = case when not flag then foo
when foo @> (select f2 from tmp) then foo
else foo || (select f2 from tmp) end
where id=6;
-- hangs on row lock
Then commit in session B, and watch A go down in flames.
Going to see if this can be narrowed down further.
--
Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Andrew> Going to see if this can be narrowed down further. Simpler testcase, removing the CTE, so this is clearly just about InitPlan: create table mytable (flag boolean default false, foo integer); insert into mytable default values; session B: begin; update mytable set flag = true; session A: update mytable set foo = case when not flag then foo else length((select 'foo')) end; commit in B and watch A die in: #1 0x0000000000b001bd in text_length (str=0) at varlena.c:647 -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Andrew> Simpler testcase, removing the CTE, so this is clearly just Andrew> about InitPlan: So I can see exactly where the problem is, but I'm not sure what the solution should be. EvalPlanQualStart copies the param_exec value list explicitly _not_ including the execPlan link, which obviously isn't going to work if the value has not been computed yet. Should it be forcing the evaluation of initplans that haven't been run yet, or should the EPQ scan evaluate them itself from a copy of the plan, or does there need to be some way to share state? (having the InitPlan be run more than once might be a problem?) -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> So I can see exactly where the problem is, but I'm not sure what the
> solution should be.
> EvalPlanQualStart copies the param_exec value list explicitly _not_
> including the execPlan link, which obviously isn't going to work if the
> value has not been computed yet. Should it be forcing the evaluation of
> initplans that haven't been run yet, or should the EPQ scan evaluate
> them itself from a copy of the plan, or does there need to be some way
> to share state? (having the InitPlan be run more than once might be a
> problem?)
The second of those; what we need is for any referenced InitPlans to be
executed afresh under EPQ rules. (I'm not entirely sure that an InitPlan
could need to see different input tuples under EPQ than it'd see
otherwise, but I'm not sure it couldn't, either.) Also, copying the
execPlan links would be bad because it'd allow EPQ to execute planstate
subtrees that are outside the portion of the planstate tree that it made
a working copy of, which doesn't seem safe (e.g., the planstates could
easily have dependencies on the particular EState they are children of).
I think that the expectation of this code was that empty execPlan
links would be filled at need during EvalPlanQualStart's ExecInitNode
calls. That's not happening because the InitPlan we're concerned about
is not attached to any plan node in the part of the tree that we copied;
it's attached to the ModifyTable node itself. We could fix that for
the particular scenario we're looking at here, perhaps by having such
initplans be initialized the same way EvalPlanQualStart treats subplans.
I'm worried though about whether any referenceable initplans might be
attached even higher in the plan tree. If we can ensure that the planner
will never do that, then a fix along these lines should be fairly
straightforward.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> So I can see exactly where the problem is, but I'm not sure what the >> solution should be. >> EvalPlanQualStart copies the param_exec value list explicitly _not_ >> including the execPlan link, which obviously isn't going to work if >> the value has not been computed yet. Should it be forcing the >> evaluation of initplans that haven't been run yet, or should the EPQ >> scan evaluate them itself from a copy of the plan, or does there >> need to be some way to share state? (having the InitPlan be run more >> than once might be a problem?) Tom> The second of those; what we need is for any referenced InitPlans Tom> to be executed afresh under EPQ rules. (I'm not entirely sure that Tom> an InitPlan could need to see different input tuples under EPQ Tom> than it'd see otherwise, but I'm not sure it couldn't, either.) Obviously you know this code better than I do... but I'm not convinced. Shouldn't the InitPlan pretty much by definition be independent of the tuples being locked/updated? And doesn't executing them again run the risk of getting a different value for other reasons, for example if an initplan is volatile? What I'm wondering is whether the param in the copied estate shouldn't rather be just a proxy for the one in the original estate - if we need to evaluate it, then do so in the original estate, store the value there, and copy the value back into the EPQ plantree. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> The second of those; what we need is for any referenced InitPlans
> Tom> to be executed afresh under EPQ rules. (I'm not entirely sure that
> Tom> an InitPlan could need to see different input tuples under EPQ
> Tom> than it'd see otherwise, but I'm not sure it couldn't, either.)
> Shouldn't the InitPlan pretty much by definition be independent of the
> tuples being locked/updated?
[ thinks for awhile... ] Yeah, I wasn't thinking clearly enough. The
point of the special EPQ rules is that, other than the target row-being-
updated, any tuples from other tables should be the *same* tuples we'd
joined that row to before EPQ. The logical extension of that to InitPlans
is that it should be the same InitPlan output as before, not potentially
a different value ...
> And doesn't executing them again run the risk of getting a different
> value for other reasons, for example if an initplan is volatile?
... and that's another good argument for not doing the initplan over.
> What I'm wondering is whether the param in the copied estate shouldn't
> rather be just a proxy for the one in the original estate - if we need
> to evaluate it, then do so in the original estate, store the value
> there, and copy the value back into the EPQ plantree.
Don't think that's going to work; the EPQ environment doesn't have any way
to know that an execPlan link is pointing to something in a different
estate.
Your other idea of forcing initPlan parameters to be evaluated before we
enter the EPQ execution environment is probably more workable. It would
be annoying to do that for every initPlan in sight, but I think we could
look at the subplan's extParam to see whether it potentially references
that parameter. (Although really, in most scenarios it wouldn't matter
because all the initPlans in a data-modifying query are probably
referenced in the subplan anyhow ...)
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> What I'm wondering is whether the param in the copied estate >> shouldn't rather be just a proxy for the one in the original estate >> - if we need to evaluate it, then do so in the original estate, >> store the value there, and copy the value back into the EPQ >> plantree. Tom> Don't think that's going to work; the EPQ environment doesn't have Tom> any way to know that an execPlan link is pointing to something in Tom> a different estate. But can't such a way be created? e.g. by pointing execPlan to a special proxy node that points back to the original estate? Tom> Your other idea of forcing initPlan parameters to be evaluated Tom> before we enter the EPQ execution environment is probably more Tom> workable. It would be annoying to do that for every initPlan in Tom> sight, but I think we could look at the subplan's extParam to see Tom> whether it potentially references that parameter. (Although Tom> really, in most scenarios it wouldn't matter because all the Tom> initPlans in a data-modifying query are probably referenced in the Tom> subplan anyhow ...) Well, the case of UPDATE ... SET foo = case when x then (select thing from big_cte) else (select thing from other_big_cte) end will be rather annoying if we end up forcing both initplans to execute. -- Andrew (irc:RhodiumToad)
I wrote:
> Your other idea of forcing initPlan parameters to be evaluated before we
> enter the EPQ execution environment is probably more workable.
Concretely, the attached seems to be enough to fix it (though I only
tried the simplest case you posted).
I don't find anything to love about ExecEvalParamExecParams: it's badly
named, badly located, full of undocumented assumptions, and probably
causes a memory leak. Plus it doesn't exist as far back as we need it
for this. But fixing those problems is a separable task. In the
meantime, this is an expedient way to test whether this approach can work.
regards, tom lane
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c583e02..35c9eb2 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 46,51 ****
--- 46,52 ----
#include "commands/matview.h"
#include "commands/trigger.h"
#include "executor/execdebug.h"
+ #include "executor/execExpr.h"
#include "foreign/fdwapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
*************** EvalPlanQualBegin(EPQState *epqstate, ES
*** 3078,3083 ****
--- 3079,3087 ----
{
int i;
+ /* First, force evaluation of any initPlans needed by subplan */
+ ExecEvalParamExecParams(planstate->plan->extParam, parentestate);
+
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
while (--i >= 0)
*************** EvalPlanQualStart(EPQState *epqstate, ES
*** 3170,3175 ****
--- 3174,3182 ----
{
int i;
+ /* First, force evaluation of any initPlans needed by subplan */
+ ExecEvalParamExecParams(planTree->extParam, parentestate);
+
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
estate->es_param_exec_vals = (ParamExecData *)
palloc0(i * sizeof(ParamExecData));
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> Don't think that's going to work; the EPQ environment doesn't have
> Tom> any way to know that an execPlan link is pointing to something in
> Tom> a different estate.
> But can't such a way be created? e.g. by pointing execPlan to a special
> proxy node that points back to the original estate?
I don't really think the amount of complexity that would add is something
to consider for a back-patchable fix.
> Well, the case of UPDATE ... SET foo = case when x then (select thing
> from big_cte) else (select thing from other_big_cte) end will be rather
> annoying if we end up forcing both initplans to execute.
Given that this bug has been there since the late bronze age and just now
got detected, I think that optimizing the fix for especially improbable
cases ought not be the first thing on our minds. Let's just get it to
work reliably.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Your other idea of forcing initPlan parameters to be evaluated
>> before we enter the EPQ execution environment is probably more
>> workable.
Tom> Concretely, the attached seems to be enough to fix it (though I
Tom> only tried the simplest case you posted).
If it helps, here is a patch that adds isolation tests to
eval-plan-qual.spec for two test cases (one with CTE, one without).
I've verified that these reproduce the crash, and that they run
successfully with your patch. I can't currently see any more specific
code paths to probe in these tests.
--
Andrew (irc:RhodiumToad)
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 9bbfdc1b5d..49b3fb3446 100644
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -184,6 +184,37 @@ ta_id ta_value tb_row
1 newTableAValue (1,tableBValue)
step c2: COMMIT;
+starting permutation: updateforcip updateforcip2 c1 c2 read_a
+step updateforcip:
+ UPDATE table_a SET value = NULL WHERE id = 1;
+
+step updateforcip2:
+ UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+ <waiting ...>
+step c1: COMMIT;
+step updateforcip2: <... completed>
+step c2: COMMIT;
+step read_a: SELECT * FROM table_a ORDER BY id;
+id value
+
+1 newValue
+
+starting permutation: updateforcip updateforcip3 c1 c2 read_a
+step updateforcip:
+ UPDATE table_a SET value = NULL WHERE id = 1;
+
+step updateforcip3:
+ WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+ UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+ <waiting ...>
+step c1: COMMIT;
+step updateforcip3: <... completed>
+step c2: COMMIT;
+step read_a: SELECT * FROM table_a ORDER BY id;
+id value
+
+1 newValue
+
starting permutation: wrtwcte readwcte c1 c2
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
step readwcte:
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 0b70ad55ba..367922de75 100644
--- a/src/test/isolation/specs/eval-plan-qual.spec
+++ b/src/test/isolation/specs/eval-plan-qual.spec
@@ -92,6 +92,13 @@ step "updateforss" {
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
}
+# these tests exercise EvalPlanQual with conditional InitPlans which
+# have not been executed prior to the EPQ
+
+step "updateforcip" {
+ UPDATE table_a SET value = NULL WHERE id = 1;
+}
+
# these tests exercise mark/restore during EPQ recheck, cf bug #15032
step "selectjoinforupdate" {
@@ -129,6 +136,13 @@ step "readforss" {
FROM table_a ta
WHERE ta.id = 1 FOR UPDATE OF ta;
}
+step "updateforcip2" {
+ UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+}
+step "updateforcip3" {
+ WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+ UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+}
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; }
step "c2" { COMMIT; }
@@ -137,6 +151,7 @@ session "s3"
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step "read" { SELECT * FROM accounts ORDER BY accountid; }
step "read_ext" { SELECT * FROM accounts_ext ORDER BY accountid; }
+step "read_a" { SELECT * FROM table_a ORDER BY id; }
# this test exercises EvalPlanQual with a CTE, cf bug #14328
step "readwcte" {
@@ -171,6 +186,8 @@ permutation "wx2" "partiallock" "c2" "c1" "read"
permutation "wx2" "lockwithvalues" "c2" "c1" "read"
permutation "wx2_ext" "partiallock_ext" "c2" "c1" "read_ext"
permutation "updateforss" "readforss" "c1" "c2"
+permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
+permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
permutation "wrtwcte" "readwcte" "c1" "c2"
permutation "wrjt" "selectjoinforupdate" "c2" "c1"
permutation "wrtwcte" "multireadwcte" "c1" "c2"
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> If it helps, here is a patch that adds isolation tests to
> eval-plan-qual.spec for two test cases (one with CTE, one without).
> I've verified that these reproduce the crash, and that they run
> successfully with your patch. I can't currently see any more specific
> code paths to probe in these tests.
Thanks! I incorporated these into the attached proposed patches.
The main difference from what I had yesterday is that I rewrote
ExecEvalParamExecParams to my satisfaction. The crucial thing I didn't
like about it was that it hard-wired use of the GetPerTupleExprContext
econtext for initplan evaluation. That seemed like it risked memory
leaks in case of repeated initplan evaluation for a single top-level
output tuple. I've since convinced myself that it's basically impossible
to leak memory in ExecSetParamPlan right now (cf comments below), but
that doesn't seem like an assumption to bake into an API when it isn't
even buying us anything to do so.
The attached is split into two parts because 0001 will need to go all
the way back, whereas 0002 only applies to HEAD and v11. I don't
plan to make them separate commits though.
A quick test says that back-patching 0001 might be slightly painful;
a lot of the hunks don't apply. I've not looked at why not.
regards, tom lane
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c583e02..0074014 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 46,51 ****
--- 46,52 ----
#include "commands/matview.h"
#include "commands/trigger.h"
#include "executor/execdebug.h"
+ #include "executor/nodeSubplan.h"
#include "foreign/fdwapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
*************** EvalPlanQualBegin(EPQState *epqstate, ES
*** 3078,3083 ****
--- 3079,3092 ----
{
int i;
+ /*
+ * Force evaluation of any InitPlan outputs that could be needed
+ * by the subplan, just in case they got reset since
+ * EvalPlanQualStart (see comments therein).
+ */
+ ExecSetParamPlanMulti(planstate->plan->extParam,
+ GetPerTupleExprContext(parentestate));
+
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
while (--i >= 0)
*************** EvalPlanQualStart(EPQState *epqstate, ES
*** 3170,3178 ****
--- 3179,3210 ----
{
int i;
+ /*
+ * Force evaluation of any InitPlan outputs that could be needed by
+ * the subplan. (With more complexity, maybe we could postpone this
+ * till the subplan actually demands them, but it doesn't seem worth
+ * the trouble; this is a corner case already, since usually the
+ * InitPlans would have been evaluated before reaching EvalPlanQual.)
+ *
+ * This will not touch output params of InitPlans that occur somewhere
+ * within the subplan tree, only those that are attached to the
+ * ModifyTable node or above it and are referenced within the subplan.
+ * That's OK though, because the planner would only attach such
+ * InitPlans to a lower-level SubqueryScan node, and EPQ execution
+ * will not descend into a SubqueryScan.
+ *
+ * The EState's per-output-tuple econtext is sufficiently short-lived
+ * for this, since it should get reset before there is any chance of
+ * doing EvalPlanQual again.
+ */
+ ExecSetParamPlanMulti(planTree->extParam,
+ GetPerTupleExprContext(parentestate));
+
+ /* now make the internal param workspace ... */
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
estate->es_param_exec_vals = (ParamExecData *)
palloc0(i * sizeof(ParamExecData));
+ /* ... and copy down all values, whether really needed or not */
while (--i >= 0)
{
/* copy value if any, but not execPlan link */
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 6b37075..63de981 100644
*** a/src/backend/executor/nodeSubplan.c
--- b/src/backend/executor/nodeSubplan.c
*************** ExecInitSubPlan(SubPlan *subplan, PlanSt
*** 1009,1014 ****
--- 1009,1025 ----
* of initplans: we don't run the subplan until/unless we need its output.
* Note that this routine MUST clear the execPlan fields of the plan's
* output parameters after evaluating them!
+ *
+ * The results of this function are stored in the EState associated with the
+ * ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
+ * result Datums are allocated in the EState's per-query memory. The passed
+ * econtext can be any ExprContext belonging to that EState; which one is
+ * important only to the extent that the ExprContext's per-tuple memory
+ * context is used to evaluate any parameters passed down to the subplan.
+ * (Thus in principle, the shorter-lived the ExprContext the better, since
+ * that data isn't needed after we return. In practice, because initplan
+ * parameters are never more complex than Vars, Aggrefs, etc, evaluating them
+ * currently never leaks any memory anyway.)
* ----------------------------------------------------------------
*/
void
*************** ExecSetParamPlan(SubPlanState *node, Exp
*** 1196,1201 ****
--- 1207,1243 ----
}
/*
+ * ExecSetParamPlanMulti
+ *
+ * Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
+ * parameters whose ParamIDs are listed in "params". Any listed params that
+ * are not initplan outputs are ignored.
+ *
+ * As with ExecSetParamPlan, any ExprContext belonging to the current EState
+ * can be used, but in principle a shorter-lived ExprContext is better than a
+ * longer-lived one.
+ */
+ void
+ ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext)
+ {
+ int paramid;
+
+ paramid = -1;
+ while ((paramid = bms_next_member(params, paramid)) >= 0)
+ {
+ ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
+
+ if (prm->execPlan != NULL)
+ {
+ /* Parameter not evaluated yet, so go do it */
+ ExecSetParamPlan(prm->execPlan, econtext);
+ /* ExecSetParamPlan should have processed this param... */
+ Assert(prm->execPlan == NULL);
+ }
+ }
+ }
+
+ /*
* Mark an initplan as needing recalculation
*/
void
diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h
index d9784a2..fd21b5d 100644
*** a/src/include/executor/nodeSubplan.h
--- b/src/include/executor/nodeSubplan.h
*************** extern void ExecReScanSetParamPlan(SubPl
*** 28,31 ****
--- 28,33 ----
extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
+ extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
+
#endif /* NODESUBPLAN_H */
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 9bbfdc1..49b3fb3 100644
*** a/src/test/isolation/expected/eval-plan-qual.out
--- b/src/test/isolation/expected/eval-plan-qual.out
*************** ta_id ta_value tb_row
*** 184,189 ****
--- 184,220 ----
1 newTableAValue (1,tableBValue)
step c2: COMMIT;
+ starting permutation: updateforcip updateforcip2 c1 c2 read_a
+ step updateforcip:
+ UPDATE table_a SET value = NULL WHERE id = 1;
+
+ step updateforcip2:
+ UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+ <waiting ...>
+ step c1: COMMIT;
+ step updateforcip2: <... completed>
+ step c2: COMMIT;
+ step read_a: SELECT * FROM table_a ORDER BY id;
+ id value
+
+ 1 newValue
+
+ starting permutation: updateforcip updateforcip3 c1 c2 read_a
+ step updateforcip:
+ UPDATE table_a SET value = NULL WHERE id = 1;
+
+ step updateforcip3:
+ WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+ UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+ <waiting ...>
+ step c1: COMMIT;
+ step updateforcip3: <... completed>
+ step c2: COMMIT;
+ step read_a: SELECT * FROM table_a ORDER BY id;
+ id value
+
+ 1 newValue
+
starting permutation: wrtwcte readwcte c1 c2
step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
step readwcte:
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 0b70ad5..367922d 100644
*** a/src/test/isolation/specs/eval-plan-qual.spec
--- b/src/test/isolation/specs/eval-plan-qual.spec
*************** step "updateforss" {
*** 92,97 ****
--- 92,104 ----
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
}
+ # these tests exercise EvalPlanQual with conditional InitPlans which
+ # have not been executed prior to the EPQ
+
+ step "updateforcip" {
+ UPDATE table_a SET value = NULL WHERE id = 1;
+ }
+
# these tests exercise mark/restore during EPQ recheck, cf bug #15032
step "selectjoinforupdate" {
*************** step "readforss" {
*** 129,134 ****
--- 136,148 ----
FROM table_a ta
WHERE ta.id = 1 FOR UPDATE OF ta;
}
+ step "updateforcip2" {
+ UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+ }
+ step "updateforcip3" {
+ WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+ UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+ }
step "wrtwcte" { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
step "wrjt" { UPDATE jointest SET data = 42 WHERE id = 7; }
step "c2" { COMMIT; }
*************** session "s3"
*** 137,142 ****
--- 151,157 ----
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step "read" { SELECT * FROM accounts ORDER BY accountid; }
step "read_ext" { SELECT * FROM accounts_ext ORDER BY accountid; }
+ step "read_a" { SELECT * FROM table_a ORDER BY id; }
# this test exercises EvalPlanQual with a CTE, cf bug #14328
step "readwcte" {
*************** permutation "wx2" "partiallock" "c2" "c1
*** 171,176 ****
--- 186,193 ----
permutation "wx2" "lockwithvalues" "c2" "c1" "read"
permutation "wx2_ext" "partiallock_ext" "c2" "c1" "read_ext"
permutation "updateforss" "readforss" "c1" "c2"
+ permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
+ permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
permutation "wrtwcte" "readwcte" "c1" "c2"
permutation "wrjt" "selectjoinforupdate" "c2" "c1"
permutation "wrtwcte" "multireadwcte" "c1" "c2"
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 9d6e25a..f597363 100644
*** a/src/backend/executor/execExprInterp.c
--- b/src/backend/executor/execExprInterp.c
*************** ExecEvalParamExec(ExprState *state, Expr
*** 2253,2285 ****
}
/*
- * ExecEvalParamExecParams
- *
- * Execute the subplan stored in PARAM_EXEC initplans params, if not executed
- * till now.
- */
- void
- ExecEvalParamExecParams(Bitmapset *params, EState *estate)
- {
- ParamExecData *prm;
- int paramid;
-
- paramid = -1;
- while ((paramid = bms_next_member(params, paramid)) >= 0)
- {
- prm = &(estate->es_param_exec_vals[paramid]);
-
- if (prm->execPlan != NULL)
- {
- /* Parameter not evaluated yet, so go do it */
- ExecSetParamPlan(prm->execPlan, GetPerTupleExprContext(estate));
- /* ExecSetParamPlan should have processed this param... */
- Assert(prm->execPlan == NULL);
- }
- }
- }
-
- /*
* Evaluate a PARAM_EXTERN parameter.
*
* PARAM_EXTERN parameters must be sought in ecxt_param_list_info.
--- 2253,2258 ----
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ee0f07a..c93084e 100644
*** a/src/backend/executor/execParallel.c
--- b/src/backend/executor/execParallel.c
***************
*** 23,29 ****
#include "postgres.h"
- #include "executor/execExpr.h"
#include "executor/execParallel.h"
#include "executor/executor.h"
#include "executor/nodeAppend.h"
--- 23,28 ----
***************
*** 36,41 ****
--- 35,41 ----
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeSeqscan.h"
#include "executor/nodeSort.h"
+ #include "executor/nodeSubplan.h"
#include "executor/tqueue.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/planmain.h"
*************** ExecInitParallelPlan(PlanState *planstat
*** 581,588 ****
char *query_string;
int query_len;
! /* Force parameters we're going to pass to workers to be evaluated. */
! ExecEvalParamExecParams(sendParams, estate);
/* Allocate object for return value. */
pei = palloc0(sizeof(ParallelExecutorInfo));
--- 581,598 ----
char *query_string;
int query_len;
! /*
! * Force any initplan outputs that we're going to pass to workers to be
! * evaluated, if they weren't already.
! *
! * For simplicity, we use the EState's per-output-tuple ExprContext here.
! * That risks intra-query memory leakage, since we might pass through here
! * many times before that ExprContext gets reset; but ExecSetParamPlan
! * doesn't normally leak any memory in the context (see its comments), so
! * it doesn't seem worth complicating this function's API to pass it a
! * shorter-lived ExprContext. This might need to change someday.
! */
! ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));
/* Allocate object for return value. */
pei = palloc0(sizeof(ParallelExecutorInfo));
*************** ExecParallelReinitialize(PlanState *plan
*** 831,838 ****
/* Old workers must already be shut down */
Assert(pei->finished);
! /* Force parameters we're going to pass to workers to be evaluated. */
! ExecEvalParamExecParams(sendParams, estate);
ReinitializeParallelDSM(pei->pcxt);
pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true);
--- 841,852 ----
/* Old workers must already be shut down */
Assert(pei->finished);
! /*
! * Force any initplan outputs that we're going to pass to workers to be
! * evaluated, if they weren't already (see comments in
! * ExecInitParallelPlan).
! */
! ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));
ReinitializeParallelDSM(pei->pcxt);
pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true);
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index f7b1f77..02af68f 100644
*** a/src/include/executor/execExpr.h
--- b/src/include/executor/execExpr.h
*************** extern void ExecEvalFuncExprStrictFusage
*** 697,703 ****
ExprContext *econtext);
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
- extern void ExecEvalParamExecParams(Bitmapset *params, EState *estate);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
--- 697,702 ----