2015-01-20 18:17 GMT+07:00 Ali Akbar <the.ap...@gmail.com>: > 2015-01-20 14:22 GMT+07:00 Jeff Davis <pg...@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 > */ >
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
