Horton

Thread

Commits

  1. Fix failure with initplans used conditionally during EvalPlanQual rechecks.

  1. Consistent segfault in complex query

    Kyle Samson <kysamson@tripadvisor.com> — 2018-09-12T14:17:25Z

    Hello,

We encountered a query that has been able to frequently segfault one of our postgres instances under certain conditions which we have not fully been able to isolate for reproduction. We were able to get a core dump out of one of the crashes and have poked at it, but we believe the answer is beyond our knowledge of postgres internals. This is on a 9.3.19 server and we saw no mention of a fix in the release notes since this version and we do not know if it affects later major releases as well.

What we know so far is that somehow the arraycontains function was given a datum of 0 as the second argument that dereferenced to a null pointer. Our current hypothesis from poking at the core dump is that some memory context is getting freed before it should. This assumption comes from the complexity in the query (CTE containing params being repeatedly evaluated 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 existence of a segfault is always concerning and we are reporting this issue in an effort to be conscientiousness members of the community.

Due to the potentially sensitive contents we cannot provide the core directly, but we are happy to run commands against the core 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=<value optimized 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

  2. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T14:55:47Z

    >>>>> "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)

  3. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-12T15:24:56Z

    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

  4. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T16:08:08Z

    >>>>> "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)

  5. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T16:13:13Z

    >>>>> "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)

  6. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T16:21:57Z

    >>>>> "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)

  7. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T16:33:22Z

    >>>>> "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)

  8. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T16:43:18Z

    >>>>> "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)

  9. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T17:21:45Z

    >>>>> "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)

  10. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-13T19:10:42Z

    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

  11. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-13T21:12:03Z

    >>>>> "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)

  12. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-13T21:51:20Z

    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

  13. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-13T22:12:22Z

    >>>>> "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)

  14. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-13T22:18:42Z

    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

  15. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-13T22:29:36Z

    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

  16. Re: Consistent segfault in complex query

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-14T02:23:34Z

    >>>>> "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)

  17. Re: Consistent segfault in complex query

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-14T19:55:40Z

    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