Re: [PATCHES] SRF memory leaks
On Tue, 2008-02-26 at 12:09 -0800, Neil Conway wrote: I'd like to apply this change to back branches reasonably soon, so if you have a better way to do the FreeTupleDesc() hack, let me know. Barring any objections, I'll apply this to HEAD and back branches tonight or tomorrow. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SRF memory leaks
Neil Conway [EMAIL PROTECTED] writes: On Tue, 2008-02-26 at 00:17 -0800, Neil Conway wrote: You didn't comment on my proposed solution (FreeTupleDesc() iff refcount == -1). I still find that entirely unsafe, particularly for something you propose to back-patch into stable branches. Negative refcount does not prove that the SRF itself hasn't still got a pointer to the tupdesc. Can't we fix it so that the tupdesc is allocated in the new special context (at least in the primary code paths), and then no explicit free is needed? regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] SRF memory leaks
On Wed, 2008-02-27 at 15:07 -0500, Tom Lane wrote: Negative refcount does not prove that the SRF itself hasn't still got a pointer to the tupdesc. That sounds quite bizarre. The SRF has already finished execution at this point, so keeping a pointer to the tupledesc around would only make sense if you wanted to use that tupledesc on a *subsequent* invocation of the SRF. The SRF would need to store the pointer in a static variable, too, and it wouldn't have an easy way to distinguish between repeated calls to the SRF within the same query and in different queries (since in the latter case the TupleDesc will be long gone anyway). I can't see why anyone would want to do this. Can't we fix it so that the tupdesc is allocated in the new special context (at least in the primary code paths), and then no explicit free is needed? As I said earlier, the tupdesc is explicitly allocated in the per-query context by the SRFs themselves. If by primary code paths, you mean SRFs in the main source tree, then sure, we can make arbitrary changes to those. That won't help out-of-tree SRFs though. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] SRF memory leaks
Neil Conway [EMAIL PROTECTED] writes: On Wed, 2008-02-27 at 15:07 -0500, Tom Lane wrote: Negative refcount does not prove that the SRF itself hasn't still got a pointer to the tupdesc. That sounds quite bizarre. The SRF has already finished execution at this point, so keeping a pointer to the tupledesc around would only make sense if you wanted to use that tupledesc on a *subsequent* invocation of the SRF. Hmm ... actually I was worried about it being embedded in the returned tuplestore, but I see tuplestore doesn't currently use a tupdesc at all, so maybe it isn't that big a problem. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] SRF memory leaks
On Mon, 2008-02-25 at 21:00 -0500, Tom Lane wrote: I find this part of the patch to be a seriously bad idea. nodeFunctionscan has no right to assume that the function has returned an expendable tupdesc; indeed, I would think that the other case is more nearly what's expected by the API for SRFs. AFAICS this is not true of any of the SRFs in the backend, which always return expendable tupdescs. A safer fix would be to try to make the tupdesc be in the new multi_call_ctx when it's being created by the funcapi.c code. funcapi.c doesn't create the tupdesc, though: it is created by user code, and merely assigned to a field of the RSI (note that the TupleDesc in question is the one in the ReturnSetInfo, not in FuncCallContext.) A minor detail is that the lifetime of the tupdesc also needs to exceed the lifetime of the multi_call_ctx, at least as currently implemented. From looking at the existing SRFs in the backend, the TupleDesc is always *explicitly* allocated in the estate's per-query context, so it will leak unless freed. Perhaps it would be sufficient to FreeTupleDesc iff tupdesc-refcount == -1? BTW, I'm thinking of changing the various SRFs that make allocations in the EState's per-query context to instead use the SRF's multi_call_ctx. This means the memory will be reclaimed sooner and avoids possible memory leaks. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] SRF memory leaks
Neil Conway [EMAIL PROTECTED] writes: On Mon, 2008-02-25 at 21:00 -0500, Tom Lane wrote: I find this part of the patch to be a seriously bad idea. AFAICS this is not true of any of the SRFs in the backend, which always return expendable tupdescs. It's OK in the built-in SRFs is disastrously different from It's OK. It was never specified that SRFs had to return a free-able tupdesc, so I think it's a lead pipe cinch that there are some out there that don't. Nor would it be their fault if we change the specification. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] SRF memory leaks
On Tue, 2008-02-26 at 03:13 -0500, Tom Lane wrote: It's OK in the built-in SRFs is disastrously different from It's OK. Right, I never said that, I was just commenting on your view that the predominant use-case for SRFs is returning refcounted tupdescs. You didn't comment on my proposed solution (FreeTupleDesc() iff refcount == -1). ISTM that we *need* to free the TupleDesc in at least some cases, in order to defend against the practice of explicitly allocating the TupleDesc in the per-query context. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] SRF memory leaks
On Tue, 2008-02-26 at 00:17 -0800, Neil Conway wrote:
You didn't comment on my proposed solution (FreeTupleDesc() iff refcount
== -1).
Attached is a revised version of this patch. It makes the
FreeTupleDesc() change described above, and fixes a bug: in
SRF_RETURN_DONE(), we need to be sure to switch back to the caller's
context before deleting the multi_call_ctx, since some SRFs (e.g.
dblink) call SRF_RETURN_DONE() while still inside the multi_call_ctx.
I'd like to apply this change to back branches reasonably soon, so if
you have a better way to do the FreeTupleDesc() hack, let me know.
-Neil
Index: src/backend/executor/nodeFunctionscan.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/executor/nodeFunctionscan.c,v
retrieving revision 1.45
diff -p -c -r1.45 nodeFunctionscan.c
*** src/backend/executor/nodeFunctionscan.c 1 Jan 2008 19:45:49 - 1.45
--- src/backend/executor/nodeFunctionscan.c 26 Feb 2008 19:04:49 -
*** FunctionNext(FunctionScanState *node)
*** 77,83
--- 77,93
* do it always.
*/
if (funcTupdesc)
+ {
tupledesc_match(node-tupdesc, funcTupdesc);
+
+ /*
+ * If it is a dynamically-allocated TupleDesc, free it: it is
+ * typically allocated in the EState's per-query context, so we
+ * must avoid leaking it on rescan.
+ */
+ if (funcTupdesc-tdrefcount == -1)
+ FreeTupleDesc(funcTupdesc);
+ }
}
/*
Index: src/backend/utils/fmgr/funcapi.c
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/utils/fmgr/funcapi.c,v
retrieving revision 1.37
diff -p -c -r1.37 funcapi.c
*** src/backend/utils/fmgr/funcapi.c 1 Jan 2008 19:45:53 - 1.37
--- src/backend/utils/fmgr/funcapi.c 26 Feb 2008 19:32:47 -
***
*** 23,28
--- 23,29
#include utils/array.h
#include utils/builtins.h
#include utils/lsyscache.h
+ #include utils/memutils.h
#include utils/syscache.h
#include utils/typcache.h
*** init_MultiFuncCall(PG_FUNCTION_ARGS)
*** 63,75
/*
* First call
*/
! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo-resultinfo;
/*
* Allocate suitably long-lived space and zero it
*/
retval = (FuncCallContext *)
! MemoryContextAllocZero(fcinfo-flinfo-fn_mcxt,
sizeof(FuncCallContext));
/*
--- 64,86
/*
* First call
*/
! ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo-resultinfo;
! MemoryContext multi_call_ctx;
!
! /*
! * Create a suitably long-lived context to hold cross-call data
! */
! multi_call_ctx = AllocSetContextCreate(fcinfo-flinfo-fn_mcxt,
! SRF multi-call context,
! ALLOCSET_SMALL_MINSIZE,
! ALLOCSET_SMALL_INITSIZE,
! ALLOCSET_SMALL_MAXSIZE);
/*
* Allocate suitably long-lived space and zero it
*/
retval = (FuncCallContext *)
! MemoryContextAllocZero(multi_call_ctx,
sizeof(FuncCallContext));
/*
*** init_MultiFuncCall(PG_FUNCTION_ARGS)
*** 81,87
retval-user_fctx = NULL;
retval-attinmeta = NULL;
retval-tuple_desc = NULL;
! retval-multi_call_memory_ctx = fcinfo-flinfo-fn_mcxt;
/*
* save the pointer for cross-call use
--- 92,98
retval-user_fctx = NULL;
retval-attinmeta = NULL;
retval-tuple_desc = NULL;
! retval-multi_call_memory_ctx = multi_call_ctx;
/*
* save the pointer for cross-call use
*** shutdown_MultiFuncCall(Datum arg)
*** 168,180
flinfo-fn_extra = NULL;
/*
! * Caller is responsible to free up memory for individual struct elements
! * other than att_in_funcinfo and elements.
*/
! if (funcctx-attinmeta != NULL)
! pfree(funcctx-attinmeta);
!
! pfree(funcctx);
}
--- 179,189
flinfo-fn_extra = NULL;
/*
! * Delete context that holds all multi-call data, including the
! * FuncCallContext itself
*/
! MemoryContextSwitchTo(flinfo-fn_mcxt);
! MemoryContextDelete(funcctx-multi_call_memory_ctx);
}
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] SRF memory leaks
Neil Conway [EMAIL PROTECTED] writes:
if (funcTupdesc)
+ {
tupledesc_match(node-tupdesc, funcTupdesc);
+ FreeTupleDesc(funcTupdesc);
+ }
}
I find this part of the patch to be a seriously bad idea.
nodeFunctionscan has no right to assume that the function has returned
an expendable tupdesc; indeed, I would think that the other case is
more nearly what's expected by the API for SRFs. We certainly daren't
backpatch such a change.
A safer fix would be to try to make the tupdesc be in the new
multi_call_ctx when it's being created by the funcapi.c code.
regards, tom lane
---(end of broadcast)---
TIP 6: explain analyze is your friend
