Re: es_query_dsa is broken

On Thu, Dec 7, 2017 at 12:51 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Wed, Dec 6, 2017 at 10:16 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Wed, Dec 6, 2017 at 1:14 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Tue, Dec 5, 2017 at 7:24 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> + EState *estate = gatherstate->ps.state;
>>>> +
>>>> + /* Install our DSA area while executing the plan. */
>>>> + estate->es_query_dsa = gatherstate->pei->area;
>>>> outerTupleSlot = ExecProcNode(outerPlan);
>>>> + estate->es_query_dsa = NULL;
>>>>
>>>> Won't the above coding pattern create a problem, if ExecProcNode
>>>> throws an error and outer block catches it and continues execution
>>>> (consider the case of execution inside PL blocks)?
>>>
>>> I don't see what the problem is. The query that got aborted by the
>>> error wouldn't be sharing an EState with one that didn't.
>>
>> That's right. Ignore my comment, I got confused. Other than that, I
>> don't see any problem with the code as such apart from that it looks
>> slightly hacky. I think Thomas or someone needs to develop a patch on
>> the lines you have mentioned or what Thomas was trying to describe in
>> his email and see how it comes out.

Andreas Seltenreich found a query[1] that suffers from this bug
(thanks!), and Amit confirmed that this patch fixes it, but further
sqlsmith fuzz testing with the patch applied also revealed that it
failed to handle the case where pei is NULL because parallelism was
inhibited. Here's a new version to fix that. Someone might prefer a
coding with "if" rather than the ternary operator, but I didn't have a
strong preference.

[1] https://www.postgresql.org/message-id/flat/CAEepm%3D2tfz1XNDcyU_a5ZiEaopz6%2BbBo9vQY%2BiJVLTtUVNztcQ%40mail.gmail.com

--
Thomas Munro
http://www.enterprisedb.com