Re: PATCH: decreasing memory needlessly consumed by array_agg

2014-12-16 10:47 GMT+07:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:
>
>
> 2014-12-16 6:27 GMT+07:00 Tomas Vondra <tv(at)fuzzy(dot)cz>:
>>
>> On 15.12.2014 22:35, Jeff Janes wrote:
>> > On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra <tv(at)fuzzy(dot)cz
>> > <mailto:tv(at)fuzzy(dot)cz>> wrote:
>> >
>> > Hi,
>> >
>> > Attached is v2 of the patch lowering array_agg memory requirements.
>> > Hopefully it addresses the issues issues mentioned by TL in this
>> thread
>> > (not handling some of the callers appropriately etc.).
>> >
>> >
>> > Hi Tomas,
>> >
>> > When configured --with-libxml I get compilation errors:
>> >
>> > xml.c: In function 'xml_xpathobjtoxmlarray':
>> > xml.c:3684: error: too few arguments to function 'accumArrayResult'
>> > xml.c:3721: error: too few arguments to function 'accumArrayResult'
>> > xml.c: In function 'xpath':
>> > xml.c:3933: error: too few arguments to function 'initArrayResult'
>> > xml.c:3936: error: too few arguments to function 'makeArrayResult'
>> >
>> > And when configured --with-perl, I get:
>> >
>> > plperl.c: In function 'array_to_datum_internal':
>> > plperl.c:1196: error: too few arguments to function 'accumArrayResult'
>> > plperl.c: In function 'plperl_array_to_datum':
>> > plperl.c:1223: error: too few arguments to function 'initArrayResult'
>> >
>> > Cheers,
>>
>> Thanks, attached is a version that fixes this.
>>
>
> Just fast-viewing the patch.
>
> The patch is not implementing the checking for not creating new context in
> initArrayResultArr. I think we should implement it also there for
> consistency (and preventing future problems).
>

Looking at the modification in accumArrayResult* functions, i don't really
comfortable with:

1. Code that calls accumArrayResult* after explicitly calling
initArrayResult* must always passing subcontext, but it has no effect.
2. All existing codes that calls accumArrayResult must be changed.

Just an idea: why don't we minimize the change in API like this:

1. Adding parameter bool subcontext, only in initArrayResult* functions
but not in accumArrayResult*
2. Code that want to not creating subcontext must calls initArrayResult*
explicitly.

Other codes that calls directly to accumArrayResult can only be changed in
the call to makeArrayResult* (with release=true parameter). In places that
we don't want to create subcontext (as in array_agg_transfn), modify it to
use initArrayResult* before calling accumArrayResult*.

What do you think?

Regards,
--
Ali Akbar