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 likeoldcontext = 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
Tom Lane wrote:
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.
Here's a patch to address Tom's SRF API memory management concerns, as
discussed earlier today on HACKERS.
Please note that, although this should apply cleanly on cvs tip, it will
have (two) failed hunks (nodeFunctionscan.c) *if* applied after Neil's
plpgsql SRF patch. Or it will cause a failure in Neil's patch if it is
applied first (I think). The fix in either case is to wrap the loop that
collects rows from the function and stashes them in the tuplestore as
follows:
do until no more tuples
+ ExprContext *econtext = scanstate->csstate.cstate.cs_ExprContext;
get one tuple
put it in the tuplestore
+ ResetExprContext(econtext);
loop
Also note that contrib/dblink is intentionally missing, because I'm
still working on other aspects of it. I'll have an updated dblink in a
day or so.
If there are no objections, please apply.
Thanks,
Joe
Attachments:
srf-api-memctx.1.patchtext/plain; name=srf-api-memctx.1.patchDownload
Index: contrib/pgstattuple/pgstattuple.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/contrib/pgstattuple/pgstattuple.c,v
retrieving revision 1.7
diff -c -r1.7 pgstattuple.c
*** contrib/pgstattuple/pgstattuple.c 23 Aug 2002 08:19:49 -0000 1.7
--- contrib/pgstattuple/pgstattuple.c 29 Aug 2002 05:31:26 -0000
***************
*** 78,94 ****
TupleDesc tupdesc;
TupleTableSlot *slot;
AttInMetadata *attinmeta;
!
! char **values;
! int i;
! Datum result;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
{
/* create a function context for cross-call persistence */
! funcctx = SRF_FIRSTCALL_INIT();
!
/* total number of tuples to be returned */
funcctx->max_calls = 1;
--- 78,97 ----
TupleDesc tupdesc;
TupleTableSlot *slot;
AttInMetadata *attinmeta;
! char **values;
! int i;
! Datum result;
! MemoryContext oldcontext;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
{
/* create a function context for cross-call persistence */
! funcctx = SRF_FIRSTCALL_INIT();
!
! /* switch to memory context appropriate for multiple function calls */
! oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
!
/* total number of tuples to be returned */
funcctx->max_calls = 1;
***************
*** 109,114 ****
--- 112,119 ----
*/
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
+
+ MemoryContextSwitchTo(oldcontext);
}
/* stuff done on every call of the function */
Index: contrib/tablefunc/tablefunc.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.2
diff -c -r1.2 tablefunc.c
*** contrib/tablefunc/tablefunc.c 15 Aug 2002 02:51:26 -0000 1.2
--- contrib/tablefunc/tablefunc.c 29 Aug 2002 05:37:30 -0000
***************
*** 87,92 ****
--- 87,93 ----
float8 stddev;
float8 carry_val;
bool use_carry;
+ MemoryContext oldcontext;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
***************
*** 94,99 ****
--- 95,103 ----
/* create a function context for cross-call persistence */
funcctx = SRF_FIRSTCALL_INIT();
+ /* switch to memory context appropriate for multiple function calls */
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
/* total number of tuples to be returned */
funcctx->max_calls = PG_GETARG_UINT32(0);
***************
*** 119,124 ****
--- 123,130 ----
* purpose it doesn't matter, just cast it as an unsigned value
*/
srandom(PG_GETARG_UINT32(3));
+
+ MemoryContextSwitchTo(oldcontext);
}
/* stuff done on every call of the function */
***************
*** 260,269 ****
AttInMetadata *attinmeta;
SPITupleTable *spi_tuptable = NULL;
TupleDesc spi_tupdesc;
! char *lastrowid;
crosstab_fctx *fctx;
int i;
int num_categories;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
--- 266,276 ----
AttInMetadata *attinmeta;
SPITupleTable *spi_tuptable = NULL;
TupleDesc spi_tupdesc;
! char *lastrowid = NULL;
crosstab_fctx *fctx;
int i;
int num_categories;
+ MemoryContext oldcontext;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
***************
*** 275,287 ****
TupleDesc tupdesc = NULL;
int ret;
int proc;
- MemoryContext oldcontext;
/* create a function context for cross-call persistence */
funcctx = SRF_FIRSTCALL_INIT();
! /* SPI switches context on us, so save it first */
! oldcontext = CurrentMemoryContext;
/* Connect to SPI manager */
if ((ret = SPI_connect()) < 0)
--- 282,293 ----
TupleDesc tupdesc = NULL;
int ret;
int proc;
/* create a function context for cross-call persistence */
funcctx = SRF_FIRSTCALL_INIT();
! /* switch to memory context appropriate for multiple function calls */
! oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* Connect to SPI manager */
if ((ret = SPI_connect()) < 0)
***************
*** 317,324 ****
SRF_RETURN_DONE(funcctx);
}
! /* back to the original memory context */
! MemoryContextSwitchTo(oldcontext);
/* get the typeid that represents our return type */
functypeid = get_func_rettype(funcid);
--- 323,330 ----
SRF_RETURN_DONE(funcctx);
}
! /* SPI switches context on us, so reset it */
! MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* get the typeid that represents our return type */
functypeid = get_func_rettype(funcid);
***************
*** 381,386 ****
--- 387,394 ----
/* total number of tuples to be returned */
funcctx->max_calls = proc;
+
+ MemoryContextSwitchTo(oldcontext);
}
/* stuff done on every call of the function */
***************
*** 432,438 ****
for (i = 0; i < num_categories; i++)
{
HeapTuple spi_tuple;
! char *rowid;
/* see if we've gone too far already */
if (call_cntr >= max_calls)
--- 440,446 ----
for (i = 0; i < num_categories; i++)
{
HeapTuple spi_tuple;
! char *rowid = NULL;
/* see if we've gone too far already */
if (call_cntr >= max_calls)
***************
*** 496,502 ****
--- 504,516 ----
xpfree(fctx->lastrowid);
if (values[0] != NULL)
+ {
+ /* switch to memory context appropriate for multiple function calls */
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
lastrowid = fctx->lastrowid = pstrdup(values[0]);
+ MemoryContextSwitchTo(oldcontext);
+ }
if (!allnulls)
{
Index: doc/src/sgml/xfunc.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql-server/doc/src/sgml/xfunc.sgml,v
retrieving revision 1.57
diff -c -r1.57 xfunc.sgml
*** doc/src/sgml/xfunc.sgml 29 Aug 2002 00:17:02 -0000 1.57
--- doc/src/sgml/xfunc.sgml 29 Aug 2002 05:30:04 -0000
***************
*** 1670,1682 ****
AttInMetadata *attinmeta;
/*
! * memory context used to initialize structure
*
! * fmctx is set by SRF_FIRSTCALL_INIT() for you, and used by
! * SRF_RETURN_DONE() for cleanup. It is primarily for internal use
! * by the API.
*/
! MemoryContext fmctx;
} FuncCallContext;
</programlisting>
--- 1670,1682 ----
AttInMetadata *attinmeta;
/*
! * memory context used for structures which must live for multiple calls
*
! * multi_call_memory_ctx is set by SRF_FIRSTCALL_INIT() for you, and used
! * by SRF_RETURN_DONE() for cleanup. It is the most appropriate memory
! * context for any memory that is allocated for use in multiple calls.
*/
! MemoryContext multi_call_memory_ctx;
} FuncCallContext;
</programlisting>
***************
*** 1720,1740 ****
Datum
my_Set_Returning_Function(PG_FUNCTION_ARGS)
{
! FuncCallContext *funcctx;
! Datum result;
[user defined declarations]
if (SRF_IS_FIRSTCALL())
{
/* one-time setup code appears here: */
[user defined code]
- funcctx = SRF_FIRSTCALL_INIT();
[if returning composite]
[build TupleDesc, and perhaps AttInMetadata]
[obtain slot]
funcctx->slot = slot;
[endif returning composite]
[user defined code]
}
/* each-time setup code appears here: */
--- 1720,1743 ----
Datum
my_Set_Returning_Function(PG_FUNCTION_ARGS)
{
! FuncCallContext *funcctx;
! Datum result;
! MemoryContext oldcontext;
[user defined declarations]
if (SRF_IS_FIRSTCALL())
{
+ funcctx = SRF_FIRSTCALL_INIT();
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* one-time setup code appears here: */
[user defined code]
[if returning composite]
[build TupleDesc, and perhaps AttInMetadata]
[obtain slot]
funcctx->slot = slot;
[endif returning composite]
[user defined code]
+ MemoryContextSwitchTo(oldcontext);
}
/* each-time setup code appears here: */
***************
*** 1747,1752 ****
--- 1750,1758 ----
{
/* here we want to return another item: */
[user defined code]
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ [user allocated memory needed in later function calls]
+ MemoryContextSwitchTo(oldcontext);
[obtain result Datum]
SRF_RETURN_NEXT(funcctx, result);
}
***************
*** 1777,1784 ****
/* stuff done only on the first call of the function */
if (SRF_IS_FIRSTCALL())
{
! /* create a function context for cross-call persistence */
! funcctx = SRF_FIRSTCALL_INIT();
/* total number of tuples to be returned */
funcctx->max_calls = PG_GETARG_UINT32(0);
--- 1783,1795 ----
/* stuff done only on the first call of the function */
if (SRF_IS_FIRSTCALL())
{
! MemoryContext oldcontext;
!
! /* create a function context for cross-call persistence */
! funcctx = SRF_FIRSTCALL_INIT();
!
! /* switch to memory context appropriate for multiple function calls */
! oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* total number of tuples to be returned */
funcctx->max_calls = PG_GETARG_UINT32(0);
***************
*** 1800,1805 ****
--- 1811,1818 ----
*/
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
+
+ MemoryContextSwitchTo(oldcontext);
}
/* stuff done on every call of the function */
Index: src/backend/executor/nodeFunctionscan.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/backend/executor/nodeFunctionscan.c,v
retrieving revision 1.6
diff -c -r1.6 nodeFunctionscan.c
*** src/backend/executor/nodeFunctionscan.c 29 Aug 2002 00:17:04 -0000 1.6
--- src/backend/executor/nodeFunctionscan.c 29 Aug 2002 04:38:01 -0000
***************
*** 96,101 ****
--- 96,102 ----
{
bool isNull;
ExprDoneCond isDone;
+ ExprContext *econtext = scanstate->csstate.cstate.cs_ExprContext;
isNull = false;
isDone = ExprSingleResult;
***************
*** 108,113 ****
--- 109,116 ----
if (isDone != ExprMultipleResult)
break;
+
+ ResetExprContext(econtext);
}
/*
Index: src/backend/utils/adt/lockfuncs.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/backend/utils/adt/lockfuncs.c,v
retrieving revision 1.3
diff -c -r1.3 lockfuncs.c
*** src/backend/utils/adt/lockfuncs.c 29 Aug 2002 00:17:05 -0000 1.3
--- src/backend/utils/adt/lockfuncs.c 29 Aug 2002 05:37:32 -0000
***************
*** 24,38 ****
Datum
pg_lock_status(PG_FUNCTION_ARGS)
{
! FuncCallContext *funccxt;
! LockData *lockData;
if (SRF_IS_FIRSTCALL())
{
- MemoryContext oldcxt;
TupleDesc tupdesc;
! funccxt = SRF_FIRSTCALL_INIT();
tupdesc = CreateTemplateTupleDesc(5, WITHOUTOID);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relation",
OIDOID, -1, 0, false);
--- 24,43 ----
Datum
pg_lock_status(PG_FUNCTION_ARGS)
{
! FuncCallContext *funcctx;
! LockData *lockData;
! MemoryContext oldcontext;
if (SRF_IS_FIRSTCALL())
{
TupleDesc tupdesc;
! /* create a function context for cross-call persistence */
! funcctx = SRF_FIRSTCALL_INIT();
!
! /* switch to memory context appropriate for multiple function calls */
! oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
!
tupdesc = CreateTemplateTupleDesc(5, WITHOUTOID);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relation",
OIDOID, -1, 0, false);
***************
*** 45,54 ****
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "isgranted",
BOOLOID, -1, 0, false);
! funccxt->slot = TupleDescGetSlot(tupdesc);
! funccxt->attinmeta = TupleDescGetAttInMetadata(tupdesc);
!
! oldcxt = MemoryContextSwitchTo(funccxt->fmctx);
/*
* Preload all the locking information that we will eventually format
--- 50,57 ----
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "isgranted",
BOOLOID, -1, 0, false);
! funcctx->slot = TupleDescGetSlot(tupdesc);
! funcctx->attinmeta = TupleDescGetAttInMetadata(tupdesc);
/*
* Preload all the locking information that we will eventually format
***************
*** 56,70 ****
* MemoryContext is reset when the SRF finishes, we don't need to
* free it ourselves.
*/
! funccxt->user_fctx = (LockData *) palloc(sizeof(LockData));
! GetLockStatusData(funccxt->user_fctx);
! MemoryContextSwitchTo(oldcxt);
}
! funccxt = SRF_PERCALL_SETUP();
! lockData = (LockData *) funccxt->user_fctx;
while (lockData->currIdx < lockData->nelements)
{
--- 59,73 ----
* MemoryContext is reset when the SRF finishes, we don't need to
* free it ourselves.
*/
! funcctx->user_fctx = (LockData *) palloc(sizeof(LockData));
! GetLockStatusData(funcctx->user_fctx);
! MemoryContextSwitchTo(oldcontext);
}
! funcctx = SRF_PERCALL_SETUP();
! lockData = (LockData *) funcctx->user_fctx;
while (lockData->currIdx < lockData->nelements)
{
***************
*** 82,88 ****
holder = &(lockData->holders[currIdx]);
lock = &(lockData->locks[currIdx]);
proc = &(lockData->procs[currIdx]);
! num_attrs = funccxt->attinmeta->tupdesc->natts;
values = (char **) palloc(sizeof(*values) * num_attrs);
--- 85,91 ----
holder = &(lockData->holders[currIdx]);
lock = &(lockData->locks[currIdx]);
proc = &(lockData->procs[currIdx]);
! num_attrs = funcctx->attinmeta->tupdesc->natts;
values = (char **) palloc(sizeof(*values) * num_attrs);
***************
*** 133,144 ****
strncpy(values[3], GetLockmodeName(mode), 32);
! tuple = BuildTupleFromCStrings(funccxt->attinmeta, values);
! result = TupleGetDatum(funccxt->slot, tuple);
! SRF_RETURN_NEXT(funccxt, result);
}
! SRF_RETURN_DONE(funccxt);
}
static LOCKMODE
--- 136,147 ----
strncpy(values[3], GetLockmodeName(mode), 32);
! tuple = BuildTupleFromCStrings(funcctx->attinmeta, values);
! result = TupleGetDatum(funcctx->slot, tuple);
! SRF_RETURN_NEXT(funcctx, result);
}
! SRF_RETURN_DONE(funcctx);
}
static LOCKMODE
Index: src/backend/utils/fmgr/funcapi.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/backend/utils/fmgr/funcapi.c,v
retrieving revision 1.3
diff -c -r1.3 funcapi.c
*** src/backend/utils/fmgr/funcapi.c 29 Aug 2002 00:17:05 -0000 1.3
--- src/backend/utils/fmgr/funcapi.c 29 Aug 2002 04:32:33 -0000
***************
*** 59,72 ****
retval->slot = NULL;
retval->user_fctx = NULL;
retval->attinmeta = NULL;
! retval->fmctx = fcinfo->flinfo->fn_mcxt;
/*
* save the pointer for cross-call use
*/
fcinfo->flinfo->fn_extra = retval;
- /* back to the original memory context */
MemoryContextSwitchTo(oldcontext);
}
else /* second and subsequent calls */
--- 59,71 ----
retval->slot = NULL;
retval->user_fctx = NULL;
retval->attinmeta = NULL;
! retval->multi_call_memory_ctx = fcinfo->flinfo->fn_mcxt;
/*
* save the pointer for cross-call use
*/
fcinfo->flinfo->fn_extra = retval;
MemoryContextSwitchTo(oldcontext);
}
else /* second and subsequent calls */
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/backend/utils/misc/guc.c,v
retrieving revision 1.85
diff -c -r1.85 guc.c
*** src/backend/utils/misc/guc.c 29 Aug 2002 00:17:05 -0000 1.85
--- src/backend/utils/misc/guc.c 29 Aug 2002 05:37:32 -0000
***************
*** 2421,2426 ****
--- 2421,2427 ----
int max_calls;
TupleTableSlot *slot;
AttInMetadata *attinmeta;
+ MemoryContext oldcontext;
/* stuff done only on the first call of the function */
if(SRF_IS_FIRSTCALL())
***************
*** 2428,2433 ****
--- 2429,2437 ----
/* create a function context for cross-call persistence */
funcctx = SRF_FIRSTCALL_INIT();
+ /* switch to memory context appropriate for multiple function calls */
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
/* need a tuple descriptor representing two TEXT columns */
tupdesc = CreateTemplateTupleDesc(2, WITHOUTOID);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
***************
*** 2450,2455 ****
--- 2454,2461 ----
/* total number of tuples to be returned */
funcctx->max_calls = GetNumConfigOptions();
+
+ MemoryContextSwitchTo(oldcontext);
}
/* stuff done on every call of the function */
Index: src/include/funcapi.h
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/include/funcapi.h,v
retrieving revision 1.5
diff -c -r1.5 funcapi.h
*** src/include/funcapi.h 29 Aug 2002 00:17:06 -0000 1.5
--- src/include/funcapi.h 29 Aug 2002 05:37:32 -0000
***************
*** 101,113 ****
AttInMetadata *attinmeta;
/*
! * memory context used to initialize structure
*
! * fmctx is set by SRF_FIRSTCALL_INIT() for you, and used by
! * SRF_RETURN_DONE() for cleanup. It is primarily for internal use
! * by the API.
*/
! MemoryContext fmctx;
} FuncCallContext;
--- 101,113 ----
AttInMetadata *attinmeta;
/*
! * memory context used for structures which must live for multiple calls
*
! * multi_call_memory_ctx is set by SRF_FIRSTCALL_INIT() for you, and used
! * by SRF_RETURN_DONE() for cleanup. It is the most appropriate memory
! * context for any memory that is allocated for use in multiple calls.
*/
! MemoryContext multi_call_memory_ctx;
} FuncCallContext;
***************
*** 160,176 ****
* {
* FuncCallContext *funcctx;
* Datum result;
* <user defined declarations>
*
* if(SRF_IS_FIRSTCALL())
* {
- * <user defined code>
* funcctx = SRF_FIRSTCALL_INIT();
* <if returning composite>
* <obtain slot>
* funcctx->slot = slot;
* <endif returning composite>
* <user defined code>
* }
* <user defined code>
* funcctx = SRF_PERCALL_SETUP();
--- 160,179 ----
* {
* FuncCallContext *funcctx;
* Datum result;
+ * MemoryContext oldcontext;
* <user defined declarations>
*
* if(SRF_IS_FIRSTCALL())
* {
* funcctx = SRF_FIRSTCALL_INIT();
+ * oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ * <user defined code>
* <if returning composite>
* <obtain slot>
* funcctx->slot = slot;
* <endif returning composite>
* <user defined code>
+ * MemoryContextSwitchTo(oldcontext);
* }
* <user defined code>
* funcctx = SRF_PERCALL_SETUP();
***************
*** 179,184 ****
--- 182,190 ----
* if (funcctx->call_cntr < funcctx->max_calls)
* {
* <user defined code>
+ * oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ * <user allocated memory needed in later function calls>
+ * MemoryContextSwitchTo(oldcontext);
* <obtain result Datum>
* SRF_RETURN_NEXT(funcctx, result);
* }
Joe Conway <mail@joeconway.com> writes:
Here's a patch to address Tom's SRF API memory management concerns, as
discussed earlier today on HACKERS.
Patch committed.
It seemed to me that pgstattuple.c does not really want to be an SRF,
but only a function returning a single tuple. As such, it can provide
a fine example of using the funcapi.h tuple-building machinery *without*
the SRF machinery. I changed it accordingly, but am not able to update
README.pgstattuple.euc_jp; Tatsuo-san, would you handle that?
regards, tom lane
Tom Lane wrote:
Patch committed.
Given the change to SRF memory management, is the following still
necessary (or possibly even incorrect)?
in funcapi.c:
per_MultiFuncCall(PG_FUNCTION_ARGS)
{
FuncCallContext *retval = (FuncCallContext *)
fcinfo->flinfo->fn_extra;
/* make sure we start with a fresh slot */
if(retval->slot != NULL)
ExecClearTuple(retval->slot);
return retval;
}
All but one of the SRFs I've tried don't seem to care, but I have one
that is getting an assertion:
0x42029331 in kill () from /lib/i686/libc.so.6
(gdb) bt
#0 0x42029331 in kill () from /lib/i686/libc.so.6
#1 0x4202911a in raise () from /lib/i686/libc.so.6
#2 0x4202a8c2 in abort () from /lib/i686/libc.so.6
#3 0x08179ab9 in ExceptionalCondition () at assert.c:48
#4 0x0818416f in pfree (pointer=0x7f7f7f7f) at mcxt.c:470
#5 0x0806bd32 in heap_freetuple (htup=0x832bb80) at heaptuple.c:736
#6 0x080e47df in ExecClearTuple (slot=0x832b2cc) at execTuples.c:406
#7 0x0817cf49 in per_MultiFuncCall (fcinfo=0xbfffe8e0) at funcapi.c:88
#8 0x40017273 in dblink_get_pkey (fcinfo=0xbfffe8e0) at dblink.c:911
Not quite sure why yet, but I'm thinking the ExecClearTuple() is no
longer needed/desired anyway.
Joe
Joe Conway <mail@joeconway.com> writes:
Given the change to SRF memory management, is the following still
necessary (or possibly even incorrect)?
/* make sure we start with a fresh slot */
if(retval->slot != NULL)
ExecClearTuple(retval->slot);
I don't think it was ever necessary... but there's nothing wrong with
it either.
All but one of the SRFs I've tried don't seem to care, but I have one
that is getting an assertion:
0x42029331 in kill () from /lib/i686/libc.so.6
(gdb) bt
#0 0x42029331 in kill () from /lib/i686/libc.so.6
#1 0x4202911a in raise () from /lib/i686/libc.so.6
#2 0x4202a8c2 in abort () from /lib/i686/libc.so.6
#3 0x08179ab9 in ExceptionalCondition () at assert.c:48
#4 0x0818416f in pfree (pointer=0x7f7f7f7f) at mcxt.c:470
#5 0x0806bd32 in heap_freetuple (htup=0x832bb80) at heaptuple.c:736
#6 0x080e47df in ExecClearTuple (slot=0x832b2cc) at execTuples.c:406
#7 0x0817cf49 in per_MultiFuncCall (fcinfo=0xbfffe8e0) at funcapi.c:88
#8 0x40017273 in dblink_get_pkey (fcinfo=0xbfffe8e0) at dblink.c:911
Not quite sure why yet, but I'm thinking the ExecClearTuple() is no
longer needed/desired anyway.
You'll need to fix that anyway because the next ExecStoreTuple will try
to do an ExecClearTuple. Looks like the same tuple is being freed
twice. Once you've handed a tuple to ExecStoreTuple it's not yours to
free anymore; perhaps some bit of code in dblink has that wrong?
regards, tom lane
Tom Lane wrote:
You'll need to fix that anyway because the next ExecStoreTuple will try
to do an ExecClearTuple. Looks like the same tuple is being freed
twice. Once you've handed a tuple to ExecStoreTuple it's not yours to
free anymore; perhaps some bit of code in dblink has that wrong?
That's just it:
0x40017273 in dblink_get_pkey (fcinfo=0xbfffe8e0) at dblink.c:911
*is*
funcctx = SRF_PERCALL_SETUP();
which is is a macro
#define SRF_PERCALL_SETUP() per_MultiFuncCall(fcinfo)
When I remove the call to ExecClearTuple() from per_MultiFuncCall(),
everything starts to work.
As you said, if the next ExecStoreTuple will try to do an
ExecClearTuple(), ISTM that it should be removed from
per_MultiFuncCall()/SRF_PERCALL_SETUP(). Or am I crazy?
Joe
Joe Conway <mail@joeconway.com> writes:
As you said, if the next ExecStoreTuple will try to do an
ExecClearTuple(), ISTM that it should be removed from
per_MultiFuncCall()/SRF_PERCALL_SETUP().
No, it's not necessary: ExecClearTuple knows the difference between a
full and an empty TupleSlot.
I'm not sure where the excess free is coming from, but it ain't
ExecClearTuple's fault. You might try setting a breakpoint at
heap_freetuple to see if that helps.
regards, tom lane
Joe Conway <mail@joeconway.com> writes:
As you said, if the next ExecStoreTuple will try to do an
ExecClearTuple(), ISTM that it should be removed from
per_MultiFuncCall()/SRF_PERCALL_SETUP(). Or am I crazy?
Actually ... on second thought ...
I bet the real issue here is that we have a long-lived TupleTableSlot
pointing at a short-lived tuple. (I assume you're just forming the
tuple in the function's working context, no?)
When ExecClearTuple is called on the next time through, it tries to
pfree a tuple that has already been recycled along with the rest of
the short-term context. Result: coredump.
However, if that were the story then *none* of the SRFs returning
tuple should work, and they do. So I'm still confused.
But I suspect that what we want to do is take management of the tuples
away from the Slot: pass should_free = FALSE to ExecStoreTuple in the
TupleGetDatum macro. The ClearTuple call *is* appropriate when you do
that, because it will reset the Slot to empty rather than leaving it
containing a dangling pointer to a long-since-freed tuple.
regards, tom lane
Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
As you said, if the next ExecStoreTuple will try to do an
ExecClearTuple(), ISTM that it should be removed from
per_MultiFuncCall()/SRF_PERCALL_SETUP(). Or am I crazy?Actually ... on second thought ...
I bet the real issue here is that we have a long-lived TupleTableSlot
pointing at a short-lived tuple. (I assume you're just forming the
tuple in the function's working context, no?)
yep
When ExecClearTuple is called on the next time through, it tries to
pfree a tuple that has already been recycled along with the rest of
the short-term context. Result: coredump.However, if that were the story then *none* of the SRFs returning
tuple should work, and they do. So I'm still confused.
That's what had me confused.
I have found the smoking gun, however. I had changed this function from
returning setof text, to returning setof
two_column_named_composite_type. *However*. I had not dropped and
recreated the function with the proper declaration. Once I redeclared
the function properly, the coredump went away, even with current
per_MultiFuncCall() code.
The way I found this was by removing ExecClearTuple() from
per_MultiFuncCall(). That allowed the function to return without core
dumping, but it gave me one column of garbage -- that finally clued me in.
But I suspect that what we want to do is take management of the tuples
away from the Slot: pass should_free = FALSE to ExecStoreTuple in the
TupleGetDatum macro. The ClearTuple call *is* appropriate when you do
that, because it will reset the Slot to empty rather than leaving it
containing a dangling pointer to a long-since-freed tuple.
OK. I'll make that change and leave ExecClearTuple() in
per_MultiFuncCall(). Sound like a plan?
Joe
Joe Conway <mail@joeconway.com> writes:
I have found the smoking gun, however. I had changed this function from
returning setof text, to returning setof
two_column_named_composite_type. *However*. I had not dropped and
recreated the function with the proper declaration. Once I redeclared
the function properly, the coredump went away, even with current
per_MultiFuncCall() code.
Ah. I think the changes I just committed would have helped:
nodeFunctionscan.c now runs a tupledesc_mismatch() check regardless of
whether it thinks the function returns RECORD or not.
But I suspect that what we want to do is take management of the tuples
away from the Slot: pass should_free = FALSE to ExecStoreTuple in the
TupleGetDatum macro. The ClearTuple call *is* appropriate when you do
that, because it will reset the Slot to empty rather than leaving it
containing a dangling pointer to a long-since-freed tuple.
OK. I'll make that change and leave ExecClearTuple() in
per_MultiFuncCall(). Sound like a plan?
First let's see if we can figure out why the code is failing to fail
as it stands. The fact that it's not dumping core says there's
something we don't understand yet ...
regards, tom lane
Joe Conway <mail@joeconway.com> writes:
Here's a patch to address Tom's SRF API memory management concerns, as
discussed earlier today on HACKERS.Patch committed.
It seemed to me that pgstattuple.c does not really want to be an SRF,
but only a function returning a single tuple.
Thank you for modifying pgstattuple.c. You are right, it does not want
to return more than 1 tuple.
As such, it can provide
a fine example of using the funcapi.h tuple-building machinery *without*
the SRF machinery. I changed it accordingly, but am not able to update
README.pgstattuple.euc_jp; Tatsuo-san, would you handle that?
Sure. I'll take care of that.
--
Tatsuo Ishii
Tatsuo Ishii wrote:
It seemed to me that pgstattuple.c does not really want to be an SRF,
but only a function returning a single tuple.Thank you for modifying pgstattuple.c. You are right, it does not want
to return more than 1 tuple.
I noticed that too, but it did occur to me that at some point you might
want to make the function return a row for every table in a database.
Perhaps even another system view (like pg_locks or pg_settings)?
Joe
Tom Lane wrote:
First let's see if we can figure out why the code is failing to fail
as it stands. The fact that it's not dumping core says there's
something we don't understand yet ...
I'm not sure if the attached will help figure it out, but at the very
least it was eye-opening for me. I ran a test on
dblink_get_pkey('foobar') that returns 5 rows. I had a breakpoint set in
ExecClearTuple. I found that ExecClearTuple was called a total of 32
times for 5 returned rows!
Relevant to this discussion was that ExecClearTuple was called three
times, with the same slot pointer, for each function call to
dblink_get_pkey. Once in SRF_PERCALL_SETUP (per_MultiFuncCall), once in
TupleGetDatum (ExecStoreTuple), and once in FunctionNext in the loop
that builds the tuplestore.
Unfortunately I have not been able to get back to a point where I see a
coredump :(. But, that did seem to be related to calling the function
with an inappropriate declaration (now it just gives me garbage instead
of dumping core, even though I reverted the per_MultiFuncCall changes I
made earlier). I'll keep messing with this for a while, but I was hoping
the attached info would lead to some more suggestions of where to be
looking.
Thanks,
Joe
Attachments:
Joe Conway wrote:
Unfortunately I have not been able to get back to a point where I see a
coredump :(. But, that did seem to be related to calling the function
with an inappropriate declaration (now it just gives me garbage instead
of dumping core, even though I reverted the per_MultiFuncCall changes I
made earlier). I'll keep messing with this for a while, but I was hoping
the attached info would lead to some more suggestions of where to be
looking.
It's back as of cvs tip. This time, it looks like all table functions
are failing in the same manner:
#4 0x081845fb in pfree (pointer=0x7f7f7f7f) at mcxt.c:470
#5 0x0806bdb2 in heap_freetuple (htup=0x82fc7b8) at heaptuple.c:736
#6 0x080e4cbf in ExecClearTuple (slot=0x82f92f4) at execTuples.c:406
#7 0x0817d3ad in per_MultiFuncCall (fcinfo=0xbfffe9e0) at funcapi.c:88
#8 0x0814af25 in pg_lock_status (fcinfo=0xbfffe9e0) at lockfuncs.c:69
#9 0x080e3990 in ExecMakeTableFunctionResult (funcexpr=0x82e9fa0,
#4 0x081845fb in pfree (pointer=0x7f7f7f7f) at mcxt.c:470
#5 0x0806bdb2 in heap_freetuple (htup=0x82f43a4) at heaptuple.c:736
#6 0x080e4cbf in ExecClearTuple (slot=0x82e9f2c) at execTuples.c:406
#7 0x0817d3ad in per_MultiFuncCall (fcinfo=0xbfffe9e0) at funcapi.c:88
#8 0x40016a4b in dblink_record (fcinfo=0xbfffe9e0) at dblink.c:518
#9 0x080e3990 in ExecMakeTableFunctionResult (funcexpr=0x82e8df8,
#4 0x081845fb in pfree (pointer=0x7f7f7f7f) at mcxt.c:470
#5 0x0806bdb2 in heap_freetuple (htup=0x83026f8) at heaptuple.c:736
#6 0x080e4cbf in ExecClearTuple (slot=0x82f71cc) at execTuples.c:406
#7 0x0817d3ad in per_MultiFuncCall (fcinfo=0xbfffe9e0) at funcapi.c:88
#8 0x08181635 in show_all_settings (fcinfo=0xbfffe9e0) at guc.c:2469
#9 0x080e3990 in ExecMakeTableFunctionResult (funcexpr=0x82f64a0,
Currently all C language table functions are broken :(, but all sql
language table functions seem to work -- which is why regression doesn't
fail (pointing out the need to add a select * from pg_settings to a
regression test somewhere).
I'm looking at this now. I suspect the easy fix is to remove
ExecClearTuple from per_MultiFuncCall, but I'll try to understand what's
going on first.
Joe
Joe Conway wrote:
I'm looking at this now. I suspect the easy fix is to remove
ExecClearTuple from per_MultiFuncCall, but I'll try to understand what's
going on first.
On second thought, *all* functions failing is what you expected, right
Tom? I just changed TupleGetDatum() as we discussed:
#define TupleGetDatum(_slot, _tuple) \
PointerGetDatum(ExecStoreTuple(_tuple, _slot, InvalidBuffer, false))
and now everything works again. Is this the preferred fix and/or is it
worth spending more time to dig into this?
Joe
Joe Conway wrote:
Joe Conway wrote:
I'm looking at this now. I suspect the easy fix is to remove
ExecClearTuple from per_MultiFuncCall, but I'll try to understand
what's going on first.On second thought, *all* functions failing is what you expected, right
Tom? I just changed TupleGetDatum() as we discussed:#define TupleGetDatum(_slot, _tuple) \
PointerGetDatum(ExecStoreTuple(_tuple, _slot, InvalidBuffer, false))and now everything works again. Is this the preferred fix and/or is it
worth spending more time to dig into this?
Here is a patch with the above mentioned fix. It also has an addition to
rangefuncs.sql and rangefuncs.out to ensure a C language table function
gets tested. I did this by adding
SELECT * FROM pg_settings WHERE name LIKE 'enable%';
to the test. I think this should produce reasonably stable results, but
obviously will require some maintenance if we add/remove a GUC variable
matching this criteria. Alternative suggestions welcomed, but if there
are no objections, please commit.
Thanks,
Joe
Attachments:
tablefunc-fix.1.patchtext/plain; name=tablefunc-fix.1.patchDownload
Index: src/include/funcapi.h
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/include/funcapi.h,v
retrieving revision 1.6
diff -c -r1.6 funcapi.h
*** src/include/funcapi.h 29 Aug 2002 17:14:33 -0000 1.6
--- src/include/funcapi.h 30 Aug 2002 17:20:49 -0000
***************
*** 148,154 ****
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, char **values);
#define TupleGetDatum(_slot, _tuple) \
! PointerGetDatum(ExecStoreTuple(_tuple, _slot, InvalidBuffer, true))
/*----------
--- 148,154 ----
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, char **values);
#define TupleGetDatum(_slot, _tuple) \
! PointerGetDatum(ExecStoreTuple(_tuple, _slot, InvalidBuffer, false))
/*----------
Index: src/test/regress/expected/rangefuncs.out
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/test/regress/expected/rangefuncs.out,v
retrieving revision 1.4
diff -c -r1.4 rangefuncs.out
*** src/test/regress/expected/rangefuncs.out 29 Aug 2002 00:17:06 -0000 1.4
--- src/test/regress/expected/rangefuncs.out 30 Aug 2002 17:29:45 -0000
***************
*** 1,3 ****
--- 1,15 ----
+ SELECT * FROM pg_settings WHERE name LIKE 'enable%';
+ name | setting
+ ------------------+---------
+ enable_hashjoin | on
+ enable_indexscan | on
+ enable_mergejoin | on
+ enable_nestloop | on
+ enable_seqscan | on
+ enable_sort | on
+ enable_tidscan | on
+ (7 rows)
+
CREATE TABLE foo2(fooid int, f2 int);
INSERT INTO foo2 VALUES(1, 11);
INSERT INTO foo2 VALUES(2, 22);
Index: src/test/regress/sql/rangefuncs.sql
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/test/regress/sql/rangefuncs.sql,v
retrieving revision 1.3
diff -c -r1.3 rangefuncs.sql
*** src/test/regress/sql/rangefuncs.sql 29 Aug 2002 00:17:06 -0000 1.3
--- src/test/regress/sql/rangefuncs.sql 30 Aug 2002 17:28:07 -0000
***************
*** 1,3 ****
--- 1,5 ----
+ SELECT * FROM pg_settings WHERE name LIKE 'enable%';
+
CREATE TABLE foo2(fooid int, f2 int);
INSERT INTO foo2 VALUES(1, 11);
INSERT INTO foo2 VALUES(2, 22);
Joe Conway <mail@joeconway.com> writes:
On second thought, *all* functions failing is what you expected, right
Tom?
Yeah. I coulda sworn I tested pg_settings yesterday after making those
other changes, but I must not have; it's sure failing for me today.
Here is a patch with the above mentioned fix. It also has an addition to
rangefuncs.sql and rangefuncs.out to ensure a C language table function
gets tested.
Good idea. Will apply.
regards, tom lane
Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
Here is a patch with the above mentioned fix. It also has an addition to
rangefuncs.sql and rangefuncs.out to ensure a C language table function
gets tested.Good idea. Will apply.
BTW, Neil, do you have a sample plpgsql table function that can be
included in the rangefuncs regression test?
Thanks,
Joe