Re: PATCH: decreasing memory needlessly consumed by array_agg

2015-01-20 14:22 GMT+07:00 Jeff Davis <pgsql(at)j-davis(dot)com>:

> The current patch, which I am evaluating for commit, does away with
> per-group memory contexts (it uses a common context for all groups), and
> reduces the initial array allocation from 64 to 8 (but preserves
> doubling behavior).

Jeff & Tomas, spotted this comment in v8 patch:
@@ -4713,6 +4733,11 @@ accumArrayResult(ArrayBuildState *astate,
/*
* makeArrayResult - produce 1-D final result of accumArrayResult
*
+ * If the array build state was initialized with a separate memory context,
+ * this also frees all the memory (by deleting the subcontext). If a parent
+ * context was used directly, the memory may be freed by an explicit
pfree()
+ * call (unless it's meant to be freed by destroying the parent context).
+ *
* astate is working state (must not be NULL)
* rcontext is where to construct result
*/

Simple pfree(astate) call is not enough to free the memory. If it's scalar
accumulation (initArrayResult), the user must pfree(astate->dvalues) and
pfree(astate->dnulls) before astate. If it's array accumulation,
pfree(astate->data) and pfree(astate->nullbitmap), with both can be null if
no array accumulated and some other cases. If its any (scalar or array)
accumulation, it's more complicated.

I suggest it's simpler to just force the API user to destroy the parent
context instead. So the comment become like this:
@@ -4713,6 +4733,11 @@ accumArrayResult(ArrayBuildState *astate,
/*
* makeArrayResult - produce 1-D final result of accumArrayResult
*
+ * If the array build state was initialized with a separate memory context,
+ * this also frees all the memory (by deleting the subcontext). If a parent
+ * context was used directly, the memory is meant to be freed by destroying
+ * the parent context.
+ *
* astate is working state (must not be NULL)
* rcontext is where to construct result
*/

Regards,
--
Ali Akbar