Re: PATCH: decreasing memory needlessly consumed by array_agg

2015-01-20 18:17 GMT+07:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:

> 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
> */
>

Sorry, there is another comment of makeMdArrayResult, i suggest also
changing it like this:
@@ -4738,6 +4764,12 @@ makeArrayResult(ArrayBuildState *astate,
* beware: no check that specified dimensions match the number of values
* accumulated.
*
+ * beware: if the astate was not initialized within a separate memory
+ * context (i.e. using subcontext=true when calling initArrayResult),
+ * using release=true is illegal as it releases the whole context,
+ * and that may include other memory still used elsewhere (instead use
+ * release=false and release the memory with the parent context later)
+ *
* astate is working state (must not be NULL)
* rcontext is where to construct result