Re: troubleshooting pointers

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> Adding a list of callbacks to
>> ExprContext seems pretty reasonable, but you'd also need some link in
>> ReturnSetInfo to let the function find the ExprContext to register
>> itself with. Then FreeExprContext would call the callbacks.

> I've made changes which fix this and will send them in with a revised
> SRF patch later today. Summary of design:
> 1.) moved the execution_state struct and ExecStatus enum to executor.h
> 2.) added "void *es" member to ExprContext
> 3.) added econtext member to ReturnSetInfo
> 4.) set rsi->econtext on the way in at ExecMakeFunctionResult()
> 5.) set rsi->econtext->es on the way in at fmgr_sql()
> 6.) used econtext->es on the way out at ExecFreeExprContext() to call
> ExecutorEnd() if needed (because postquel_execute() never got the chance).

Um. I don't like that; it assumes not only that ExecutorEnd is the only
kind of callback needed, but also that there is at most one function
per ExprContext that needs a shutdown callback. Neither of these
assumptions hold water IMO.

The design I had in mind was more like this: add to ExprContext a list
header field pointing to a list of structs along the lines of

struct exprcontext_callback {
struct exprcontext_callback *next;
void (*function) (Datum);
Datum arg;
}

and then call each specified function with given argument during
FreeExprContext. Probably ought to be careful to do that in reverse
order of registration. We'd also need to invent a RescanExprContext
operation to call the callbacks during a Rescan. The use of Datum
(and not, say, void *) as PG's standard callback arg type was settled on
some time ago --- originally for on_proc_exit IIRC --- and seems to have
worked well enough.

>> Hmm ... another advantage of doing this is that the function would be
>> able to find the ecxt_per_query_memory associated with the ExprContext.
>> That would be a Good Thing.

> What does this allow done that can't be done today?

It provides a place for the function to allocate stuff that needs to
live over multiple calls, ie, until it gets its shutdown callback.
Right now a function has to use TransactionCommandContext for that,
but that's really too coarse-grained.

>> We should also think about the fcache (FunctionCache) struct and whether
>> that needs to tie into this. See the FIXME in utils/fcache.h.

> While I was at it, I added an fcache member to ExprContext, and
> populated it in ExecMakeFunctionResult() for SRF cases. I wasn't sure
> what else to do with it at the moment, but at least it is a step in the
> right direction.

Well, I was debating whether that's good or not. The existing fcache
approach is wrong (per cited FIXME); it might be better not to propagate
access of it into more places. Unless you can see a specific reason to
allow the function to have access to the fcache struct, I think I'm
inclined not to.

What's really more relevant here is that during the hypothetical new
RescanExprContext function, we ought to go around and clear any fcaches
in the context that have setArgsValid = true, so that they will be
restarted afresh during the next scan of the plan. (The fact that that
doesn't happen now is another shortcoming of the existing set-functions-
in-expressions code.) So this suggests making a callback function type
specifically to do that, and registering every fcache that is executing
a set function in the callback list...

regards, tom lane