Re: PATCH: decreasing memory needlessly consumed by array_agg

>     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.

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