Re: PATCH: decreasing memory needlessly consumed by array_agg - Mailing list pgsql-hackers

Hi!

First of all, thanks for the review - the insights and comments are
spot-on. More comments below.

On 20.12.2014 09:26, Ali Akbar wrote:
>
> 2014-12-16 11:01 GMT+07:00 Ali Akbar <the.apaan@gmail.com
> <mailto:the.apaan@gmail.com>>:
>
>
>
>     2014-12-16 10:47 GMT+07:00 Ali Akbar <the.apaan@gmail.com
>     <mailto:the.apaan@gmail.com>>:
>
>
>         2014-12-16 6:27 GMT+07:00 Tomas Vondra <tv@fuzzy.cz
>         <mailto:tv@fuzzy.cz>>:
>         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).

You're right that initArrayResultArr was missing the code deciding
whether to create a subcontext or reuse the parent one, and the fix you
proposed (i.e. reusing code from initArrayResult) is IMHO the right one.

> Testing the performance with your query, looks promising: speedup is
> between 12% ~ 15%.
>
> Because i'm using 32-bit systems, setting work_mem to 1024GB failed:
>
>     ERROR:  1073741824 is outside the valid range for parameter
>     "work_mem" (64 .. 2097151)
>     STATEMENT:  SET work_mem = '1024GB';
>     psql:/media/truecrypt1/oss/postgresql/postgresql/../patches/array-agg.sql:20:
>     ERROR:  1073741824 is outside the valid range for parameter
>     "work_mem" (64 .. 2097151)

Yes, that's pretty clearly because of the 2GB limit on 32-bit systems.

> Maybe because of that, in the large groups a test, the speedup is awesome:
>
>     master: 16,819 ms
>
>     with patch: 1,720 ms

Probably. It's difficult to say without explain plans or something, but
it's probably using a different plan (e.g. group aggregate).

> Looks like with master, postgres resort to disk, but with the patch it
> fits in memory.

I'd bet that's not postgres, but system using a swap (because postgres
allocates a lot of memory).

> Note: I hasn't tested the large dataset.
>
> As expected, testing array_agg(anyarray), the performance is still the
> same, because the subcontext hasn't implemented there (test script
> modified from Tomas', attached).
>
> I implemented the subcontext checking in initArrayResultArr by changing
> the v3 patch like this:
>
>     +++ b/src/backend/utils/adt/arrayfuncs.c
>     @@ -4797,10 +4797,11 @@ initArrayResultArr(Oid array_type, Oid
>     element_type, MemoryContext rcontext,
>                        bool subcontext)
>      {
>         ArrayBuildStateArr *astate;
>     -   MemoryContext arr_context;
>     +   MemoryContext arr_context = rcontext;   /* by default use the
>     parent ctx */
>
>         /* Make a temporary context to hold all the junk */
>     -   arr_context = AllocSetContextCreate(rcontext,
>     +   if (subcontext)
>     +       arr_context = AllocSetContextCreate(rcontext,
>                                             "accumArrayResultArr",
>                                             ALLOCSET_DEFAULT_MINSIZE,
>                                             ALLOCSET_DEFAULT_INITSIZE,
>
>
> Testing the performance, it got the 12%~15% speedup. Good. (patch attached)

Nice, and it's consistent with my measurements on scalar values.

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

I think it's an interesting idea.

I've been considering this before, when thinking about the best way to
keep the calls to the various methods consistent (eg. enforcing the use
of release=true only with subcontexts).

What I ended up doing (see the v4 patch attached) is that I

  (1) added 'private_cxt' flag to the ArrayBuildState[Arr] struct,
      tracking whether there's a private memory context

  (2) rolled back all the API changes, except for the initArray*
      methods (as you proposed)

This has the positive benefit that it allows checking consistency of the
calls - you can still do

   initArrayResult(..., subcontext=false)
   ...
   makeArrayResult(..., release=true)

but it won't reset the memory context, and with assert-enabled build it
will actually fail.

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


> As per your concern about calling initArrayResult* with
> subcontext=false, while makeArrayResult* with release=true:
>
>       Also, it seems that using 'subcontext=false' and then 'release=true'
>       would be a bug. Maybe it would be appropriate to store the
>       'subcontext' value into the ArrayBuildState and then throw an error
>       if makeArrayResult* is called with (release=true && subcontext=false).
>
>
> Yes, i think we should do that to minimize unexpected coding errors.
> In makeArrayResult*, i think its better to not throwing an error, but
> using assertions:
>
>     Assert(release == false || astate->subcontext == true);

Yes. I called the flag 'private_cxt' but that's a minor difference. The
assert I used is this:

    /* we can only release the context if it's a private one. */
    Assert(! (release && !astate->private_cxt));

regards
Tomas