Re: es_query_dsa is broken

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.

Yeah, it is a bit hacky, but I can't see a another way to fix it
without changing released APIs and it's only for one release and will
certainly be unhackified in v11. For v11 I think we need to decide
between:

1. Removing es_query_dsa and injecting the right context into the
executor tree as discussed.

2. Another idea mentioned by Robert in an off-list chat: We could
consolidate all DSM segments in a multi-gather plan into one. See the
nearby thread where someone had over a hundred Gather nodes and had to
crank up max_connections to reserve enough DSM slots. Of course,
optimising for that case doesn't make too much sense (I suspect
multi-gather plans will become less likely with Parallel Append and
Parallel Hash in the picture anyway), but it would reduce a bunch of
duplicated work we do when it happens as well as scarce slot
consumption. If we did that, then all of a sudden es_query_dsa would
make sense again (ie it'd be whole-query scoped), and we could revert
that hacky change.

Or we could do both things anyway.

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