Обсуждение: Concern about memory management with SRFs

I've been looking at the sample SRFs (show_all_settings etc) and am not
happy about the way memory management is done.  As the code is currently
set up, the functions essentially assume that they are executed in a
context that will never be reset until they're done returning tuples.
(This is true because tupledescs and so on are blithely constructed in
CurrentMemoryContext during the first call.)

This approach means that SRFs cannot afford to leak any memory per-call.
If they do, and the result set is large, they will run the backend out
of memory.  I don't think that's acceptable.

The reason that the code fails to crash is that nodeFunctionscan.c
doesn't do a ResetExprContext(econtext) in the loop that collects rows
from the function and stashes them in the tuplestore.  But I think it
must do so in the long run, and so it would be better to get this right
the first time.

I think we should document that any memory that is allocated in the
first call for use in subsequent calls must come from the memory context
saved in FuncCallContext (and let's choose a more meaningful name than
fmctx, please).  This would mean adding code like
oldcontext = MemoryContextSwitchTo(funcctx->fmctx);
...
MemoryContextSwitchTo(oldcontext);

around the setup code that follows SRF_FIRSTCALL_INIT.  Then it would be
safe for nodeFunctionscan.c to do a reset before each function call.

Comments?
        regards, tom lane

Tom Lane wrote:
> I think we should document that any memory that is allocated in the
> first call for use in subsequent calls must come from the memory context
> saved in FuncCallContext (and let's choose a more meaningful name than
> fmctx, please).  This would mean adding code like
> 
>     oldcontext = MemoryContextSwitchTo(funcctx->fmctx);
> 
>     ...
> 
>     MemoryContextSwitchTo(oldcontext);
> 
> around the setup code that follows SRF_FIRSTCALL_INIT.  Then it would be
> safe for nodeFunctionscan.c to do a reset before each function call.

That sounds like a good plan.

But can/should we wrap those calls in either existing or new macros? Or 
is it better to have the function author keenly aware of the memory 
management details? I tend to think the former is better.

Maybe SRF_FIRSTCALL_INIT()(init_MultiFuncCall()) should:
- save CurrentMemoryContext to funcctx->per_call_memory_ctx  (new member of the struct)
- save fcinfo->flinfo->fn_mcxt to funcctx->multi_call_memory_ctx  (nee funcctx->fmctx)
- leave fcinfo->flinfo->fn_mcxt as the current memory context when it  exits

Then SRF_PERCALL_SETUP() (per_MultiFuncCall()) can change back to 
funcctx->per_call_memory_ctx.

Would this work?

Joe

Joe Conway <mail@joeconway.com> writes:
> Maybe SRF_FIRSTCALL_INIT()(init_MultiFuncCall()) should:
> - save CurrentMemoryContext to funcctx->per_call_memory_ctx
>    (new member of the struct)
> - save fcinfo->flinfo->fn_mcxt to funcctx->multi_call_memory_ctx
>    (nee funcctx->fmctx)
> - leave fcinfo->flinfo->fn_mcxt as the current memory context when it
>    exits

> Then SRF_PERCALL_SETUP() (per_MultiFuncCall()) can change back to 
> funcctx->per_call_memory_ctx.

I thought about that and didn't like it; it may simplify the simple case
but I think it actively gets in the way of less-simple cases.  For
example, the FIRSTCALL code might generate some transient structures
along with ones that it wants to keep.  Also, your recommended
pseudocode allows the author to write code between the end of the
FIRSTCALL branch and the PERCALL_SETUP call; that code will not execute
in a predictable context if we do it this way.

I'm also not happy with the implied assumption that every call to the
function executes in the same transient context.  That is true at the
moment but I'd just as soon not see it as a wired-in assumption.
        regards, tom lane

Tom Lane wrote:
> I thought about that and didn't like it; it may simplify the simple case
> but I think it actively gets in the way of less-simple cases.  For
> example, the FIRSTCALL code might generate some transient structures
> along with ones that it wants to keep.  Also, your recommended
> pseudocode allows the author to write code between the end of the
> FIRSTCALL branch and the PERCALL_SETUP call; that code will not execute
> in a predictable context if we do it this way.
> 
> I'm also not happy with the implied assumption that every call to the
> function executes in the same transient context.  That is true at the
> moment but I'd just as soon not see it as a wired-in assumption.

Fair enough. I'll take a shot at the necessary changes (if you want me 
to). Is it OK to use fcinfo->flinfo->fn_mcxt as the long term memory 
context or is there a better choice? Is funcctx->multi_call_memory_ctx a 
suitable name in place of funcctx->fmctx?

Joe

Joe Conway <mail@joeconway.com> writes:
> Is it OK to use fcinfo->flinfo->fn_mcxt as the long term memory 
> context or is there a better choice?

That is the correct choice.

> Is funcctx->multi_call_memory_ctx a 
> suitable name in place of funcctx->fmctx?

No objection here.
        regards, tom lane