Re: PATCH: decreasing memory needlessly consumed by array_agg

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

> Also, what is it with this hunk?
>
>> @@ -4768,6 +4770,9 @@ makeMdArrayResult(ArrayBuildState *astate,
>>
>>      MemoryContextSwitchTo(oldcontext);
>>
>> +    /* we can only release the context if it's a private one. */
>> +    // Assert(! (release && !astate->private_cxt));
>> +
>>      /* Clean up all the junk */
>>      if (release)
>>          MemoryContextDelete(astate->mcontext);

That's a mistake, of couse - the assert should not be commented out.

Attached is v6 of the patch, with the comments and assert fixed.


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.

regards
Tomas