> >     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.
> I'm wondering whether this is necessary after fixing makeArrayResult to
> use the privat_cxt flag. It's still possible to call makeMdArrayResult
> directly (with the wrong 'release' value).
> Another option might be to get rid of the 'release' flag altogether, and
> just use the 'private_cxt' - I'm not aware of a code using release=false
> with private_cxt=true (e.g. to build the same array twice from the same
> astate). But maybe there's such code, and another downside is that it'd
> break the existing API.
> >     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.
> Yeah, although I'd much rather not break the existing code at all. That
> is - my goal is not to make it slower unless absolutely necessary (and
> in that case the code may be fixed per your suggestion). But I'm not
> convinced it's worth it.

OK. Do you think we need to note this in the comments? Something like this:
If using subcontext=false, the caller must be careful about memory usage,
because makeArrayResult* will not free the memory used.

> But I think it makes sense to move the error handling into
> initArrayResultArr(), including the get_element_type() call, and remove
> the element_type from the signature. This means initArrayResultAny()
> will call the get_element_type() twice, but I guess that's negligible.
> And everyone who calls initArrayResultArr() will get the error handling
> for free.
> Patch v7 attached, implementing those two changes, i.e.
>   * makeMdArrayResult(..., astate->private_cxt)
>   * move error handling into initArrayResultArr()
>   * remove element_type from initArrayResultArr() signature

Reviewing the v7 patch:
- applies cleanly to current master. patch format, whitespace, etc is good
- make check runs without error
- performance & memory usage still consistent

If you think we don't have to add the comment (see above), i'll mark this
as ready for committer

Ali Akbar

Reply via email to