> > Another positive benefit is that this won't break the code unless it > uses the new API. This is a problem especially with external code (e.g. > extensions), but the new API (initArray*) is not part of 9.4 so there's > no such code. So that's nice. > > The one annoying thing is that this makes the API slighly unbalanced. > With the new API you can use a shared memory context, which with the old > one (not using the initArray* methods) you can't. > > But I'm OK with that, and it makes the patch smaller (15kB -> 11kB).
Yes, with this API, we can backpatch this patch to 9.4 (or below) if we need it there. I think this API is a good compromise of old API and new API. Ideally if we can migrate all code to new API (all code must call initArrayResult* before accumArrayResult*), we can remove parameter MemoryContext rcontext from accumArrayResult. Currently, the code isn't using the rcontext for anything except for old API calls (in first call to accumArrayResult). 2014-12-21 20:38 GMT+07:00 Tomas Vondra <t...@fuzzy.cz>: > On 21.12.2014 02:54, Alvaro Herrera wrote: > > Tomas Vondra wrote: > >> Attached is v5 of the patch, fixing an error with releasing a shared > >> memory context (invalid flag values in a few calls). > > > > The functions that gain a new argument should get their comment updated, > > to explain what the new argument is for. > > Right. I've added a short description of the 'subcontext' parameter to > all three variations of the initArray* function, and a more thorough > explanation to initArrayResult(). > With this API, i think we should make it clear if we call initArrayResult with subcontext=false, we can't call makeArrayResult, but we must use makeMdArrayResult directly. Or better, we can modify makeArrayResult to release according to astate->private_cxt: > @@ -4742,7 +4742,7 @@ makeArrayResult(ArrayBuildState *astate, > dims[0] = astate->nelems; > lbs[0] = 1; > > - return makeMdArrayResult(astate, ndims, dims, lbs, rcontext, true); > + return makeMdArrayResult(astate, ndims, dims, lbs, rcontext, > astate->private_cxt); > Or else we implement what you suggest below (more comments below): Thinking about the 'release' flag a bit more - maybe we could do this > instead: > > if (release && astate->private_cxt) > MemoryContextDelete(astate->mcontext); > else if (release) > { > pfree(astate->dvalues); > pfree(astate->dnulls); > pfree(astate); > } > > i.e. either destroy the whole context if possible, and just free the > memory when using a shared memory context. But I'm afraid this would > penalize the shared memory context, because that's intended for cases > where all the build states coexist in parallel and then at some point > are all converted into a result and thrown away. Adding pfree() calls is > no improvement here, and just wastes cycles. > As per Tom's comment, i'm using "parent memory context" instead of "shared memory context" below. In the future, if some code writer decided to use subcontext=false, to save memory in cases where there are many array accumulation, and the parent memory context is long-living, current code can cause memory leak. So i think we should implement your suggestion (pfreeing astate), and warn the implication in the API comment. The API user must choose between release=true, wasting cycles but preventing memory leak, or managing memory from the parent memory context. In one possible use case, for efficiency maybe the caller will create a special parent memory context for all array accumulation. Then uses makeArrayResult* with release=false, and in the end releasing the parent memory context once for all. Regards, -- Ali Akbar