Re: PATCH: decreasing memory needlessly consumed by array_agg

2015-01-20 14:22 GMT+07:00 Jeff Davis <pgsql@j-davis.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

  */