Thread: 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