Thread: [HACKERS] ExecPrepareExprList and per-query context

As of b8d7f053c5c, ExecPrepareExprList is (must be?) used instead of
ExecPrepareExpr when the caller wants to initialize expressions in a list,
for example, FormIndexDatum.  ExecPrepareExpr doesn't require the caller
to have switched to per-query context, because it itself will.  Same is
not however true for the new ExecPrepareExprList.  That means the List
node that it creates might be in a context that is not necessarily
per-query context, where it previously would be.  That breaks third-party
users of FormIndexDatum that rely on the list to have been created in
per-query context (pg_bulkload was broken by this).

Should ExecPrepareExprList also switch to estate->es_query_cxt?  Or maybe
ExecPrepareExpr could itself detect that passed-in node is a List and
create the list of ExprState nodes by itself.  I guess the reason to
separate list case is because ExecInitExpr() does not take Lists anymore.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> Should ExecPrepareExprList also switch to estate->es_query_cxt?

Good point; I'm surprised we haven't noted any failures from that.
We surely want the entire result data structure to be in the same
memory context.  There are not very many callers right now, and
I guess they are all in the right context already (or we aren't
testing them :-().

> Or maybe
> ExecPrepareExpr could itself detect that passed-in node is a List and
> create the list of ExprState nodes by itself.

-1.  That's just breaking the API of ExecPrepareExpr.
        regards, tom lane

On 2017/04/08 1:49, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> Should ExecPrepareExprList also switch to estate->es_query_cxt?
> 
> Good point; I'm surprised we haven't noted any failures from that.
> We surely want the entire result data structure to be in the same
> memory context.  There are not very many callers right now, and
> I guess they are all in the right context already (or we aren't
> testing them :-().

Thanks for the fix.

>> Or maybe
>> ExecPrepareExpr could itself detect that passed-in node is a List and
>> create the list of ExprState nodes by itself.
> 
> -1.  That's just breaking the API of ExecPrepareExpr.

I guess you're right.  I was just thinking that passing a List through
ExecPrepareExpr() used to work and now it doesn't.

Thanks,
Amit