Thread: PATCH: decreasing memory needlessly consumed by array_agg

Hi,

this is a patch for issue reported in October 2013 in pgsql-bugs:

  http://www.postgresql.org/message-id/3839201.Nfa2RvcheX@techfox.foxi

Frank van Vugt reported that a simple query with array_agg() and large
number of groups (1e7) fails because of OOM even on machine with 32GB of
RAM.

So for example doing this:

   CREATE TABLE test (a INT, b INT);
   INSERT INTO test SELECT i, i FROM generate_series(1,10000000) s(i);

   SELECT a, array_agg(b) FROM test GROUP BY a;

allocates huge amounts of RAM and easily forces the machine into
swapping and eventually gets killed by OOM (on my workstation with 8GB
of RAM that happens almost immediately).

Upon investigation, it seems caused by a combination of issues:

(1) per-group memory contexts - each group state uses a dedicated
    memory context, which is defined like this (in accumArrayResult):

    arr_context = AllocSetContextCreate(rcontext,
                "accumArrayResult",
                ALLOCSET_DEFAULT_MINSIZE,
                ALLOCSET_DEFAULT_INITSIZE,
                ALLOCSET_DEFAULT_MAXSIZE);

    which actually means this

    arr_context = AllocSetContextCreate(rcontext,
                "accumArrayResult",
                0,
                (8*1024),
                (8*1024*1024));

    so each group will allocate at least 8kB of memory (of the first
    palloc call). With 1e7 groups, that's ~80GB of RAM, even if each
    group contains just 1 item.

(2) minimum block size in aset.c - The first idea I got was to decrease
    the block size in the allocator. So I decreased it to 256B but I
    was still getting OOM. Then I found that aset.c contains this:

        if (initBlockSize < 1024)
        initBlockSize = 1024;

    so effectively the lowest allowed block size is 1kB. Which means
    ~10GB of memory for the state data (i.e. not considering overhead
    of the hash table etc., which is not negligible).

    Considering we're talking about 1e7 32-bit integers, i.e. 40MB
    of raw data, that's still pretty excessive (250x more).

While I question whether the 1kB minimum block size makes sense, I
really think per-group memory contexts don't make much sense here. What
is the point of per-group memory contexts?

The memory will be allocated when the first row of the group is
received, and won't be allocated until the whole result set is
processed. At least that's how it works for Hash Aggregate.

However that's exactly how it would work with a single memory context,
which has the significant benefit that all the groups share the same
memory (so the minimum block size is not an issue).

That is exactly what the patch aims to do - it removes the per-group
memory contexts and reuses the main memory context of the aggregate
itself.

The patch also does one more thing - it changes how the arrays (in the
aggregate state) grow. Originally it worked like this

    /* initial size */
    astate->alen = 64;

    /* when full, grow exponentially */
    if (astate->nelems >= astate->alen)
        astate->alen *= 2;

so the array length would grow like this 64 -> 128 -> 256 -> 512 ...
(note we're talking about elements, not bytes, so with with 32-bit
integers it's actually 256B -> 512B -> 1024B -> ...).

While I do understand the point of this (minimizing palloc overhead), I
find this pretty dangerous, especially in case of array_agg(). I've
modified the growth like this:

    /* initial size */
    astate->alen = 4;

    /* when full, grow exponentially */
    if (astate->nelems >= astate->alen)
        astate->alen += 4;

I admit that might be a bit too aggressive, and maybe there's a better
way to do this - with better balance between safety and speed. I was
thinking about something like this:


    /* initial size */
    astate->alen = 4;

    /* when full, grow exponentially */
    if (astate->nelems >= astate->alen)
        if (astate->alen < 128)
            astate->alen *= 2;
        else
            astate->alen += 128;

i.e. initial size with exponential growth, but capped at 128B.


regards
Tomas

On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
> The patch also does one more thing - it changes how the arrays (in the
> aggregate state) grow. Originally it worked like this
>
>     /* initial size */
>     astate->alen = 64;
>
>     /* when full, grow exponentially */
>     if (astate->nelems >= astate->alen)
>         astate->alen *= 2;
>
> so the array length would grow like this 64 -> 128 -> 256 -> 512 ...
> (note we're talking about elements, not bytes, so with with 32-bit
> integers it's actually 256B -> 512B -> 1024B -> ...).
>
> While I do understand the point of this (minimizing palloc overhead), I
> find this pretty dangerous, especially in case of array_agg(). I've
> modified the growth like this:
>
>     /* initial size */
>     astate->alen = 4;
>
>     /* when full, grow exponentially */
>     if (astate->nelems >= astate->alen)
>         astate->alen += 4;
>
> I admit that might be a bit too aggressive, and maybe there's a better
> way to do this - with better balance between safety and speed. I was
> thinking about something like this:
>
>
>     /* initial size */
>     astate->alen = 4;
>
>     /* when full, grow exponentially */
>     if (astate->nelems >= astate->alen)
>         if (astate->alen < 128)
>             astate->alen *= 2;
>         else
>             astate->alen += 128;
>
> i.e. initial size with exponential growth, but capped at 128B.

So I think this kind of thing is very sensible, but the last time I
suggested something similar, I got told "no":

http://www.postgresql.org/message-id/CAEYLb_WLGHT7yJLaRE9PPeRt5RKd5ZJbb15tE+kpgejgQKORyA@mail.gmail.com

But I think you're right and the objections previously raised are
wrong.  I suspect that the point at which we should stop doubling is
higher than 128 elements, because that's only 8kB, which really isn't
that big - and the idea that the resizing overhead takes only
amortized constant time is surely appealing.  But I still think that
doubling *forever* is a bad idea, here and there.  The fact that we've
written the code that way in lots of places doesn't make it the right
algorithm.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 31.3.2014 21:04, Robert Haas wrote:
> On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
>> The patch also does one more thing - it changes how the arrays (in the
>> aggregate state) grow. Originally it worked like this
>>
>>     /* initial size */
>>     astate->alen = 64;
>>
>>     /* when full, grow exponentially */
>>     if (astate->nelems >= astate->alen)
>>         astate->alen *= 2;
>>
>> so the array length would grow like this 64 -> 128 -> 256 -> 512 ...
>> (note we're talking about elements, not bytes, so with with 32-bit
>> integers it's actually 256B -> 512B -> 1024B -> ...).
>>
>> While I do understand the point of this (minimizing palloc overhead), I
>> find this pretty dangerous, especially in case of array_agg(). I've
>> modified the growth like this:
>>
>>     /* initial size */
>>     astate->alen = 4;
>>
>>     /* when full, grow exponentially */
>>     if (astate->nelems >= astate->alen)
>>         astate->alen += 4;
>>
>> I admit that might be a bit too aggressive, and maybe there's a better
>> way to do this - with better balance between safety and speed. I was
>> thinking about something like this:
>>
>>
>>     /* initial size */
>>     astate->alen = 4;
>>
>>     /* when full, grow exponentially */
>>     if (astate->nelems >= astate->alen)
>>         if (astate->alen < 128)
>>             astate->alen *= 2;
>>         else
>>             astate->alen += 128;
>>
>> i.e. initial size with exponential growth, but capped at 128B.
> 
> So I think this kind of thing is very sensible, but the last time I
> suggested something similar, I got told "no":
> 
> http://www.postgresql.org/message-id/CAEYLb_WLGHT7yJLaRE9PPeRt5RKd5ZJbb15tE+kpgejgQKORyA@mail.gmail.com
> 
> But I think you're right and the objections previously raised are 
> wrong. I suspect that the point at which we should stop doubling is 
> higher than 128 elements, because that's only 8kB, which really
> isn't that big - and the idea that the resizing overhead takes only 
> amortized constant time is surely appealing. But I still think that 
> doubling *forever* is a bad idea, here and there. The fact that
> we've written the code that way in lots of places doesn't make it the
> right algorithm.

I've been thinking about it a bit more and maybe the doubling is not
that bad idea, after all. What I'd like to see is a solution that does
"wastes" less than some known fraction of the allocated memory, and
apparently that's what doubling does ...

Let's assume we have many buffers (arrays in array_agg), allocated in
this manner. Let's assume the buffers are independent, i.e. the doubling
is not somehow "synchronized" for the buffers.

Now, at arbitrary time the buffers should be ~75% full on average. There
will be buffers that were just doubled (50% full), buffers that will be
doubled soon (100% full) and buffers somewhere in between. But on
average the buffers should be 75%. That means we're "wasting" 25% memory
on average, which seems quite acceptable to me. We could probably use a
different growth rate (say 1.5x, resulting in 12.5% memory being
"wasted"), but I don't see this as the main problem (and I won't fight
for this part of array_agg patch).

The "current" array_agg however violates some of the assumptions
mentioned above, because it

(1) pre-allocates quite large number of items (64) at the beginning,   resulting in ~98% of memory being "wasted"
initially

(2) allocates one memory context per group, with 8kB initial size, so   you're actually wasting ~99.999% of the memory

(3) thanks to the dedicated memory contexts, the doubling is pretty   much pointless up until you cross the 8kB
boundary

IMNSHO these are the issues we really should fix - by lowering the
initial element count (64->4) and using a single memory context.

regards
Tomas

How much of this problem can be attributed by the fact that repalloc has
to copy the data from the old array into the new one?  If it's large,
perhaps we could solve it by replicating the trick we use for
InvalidationChunk.  It'd be a bit messy, but the mess would be pretty
well contained, I think.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Tomas Vondra <tv@fuzzy.cz> writes:
> I've been thinking about it a bit more and maybe the doubling is not
> that bad idea, after all.

It is not.  There's a reason why that's our standard behavior.

> The "current" array_agg however violates some of the assumptions
> mentioned above, because it
> (1) pre-allocates quite large number of items (64) at the beginning,
>     resulting in ~98% of memory being "wasted" initially
> (2) allocates one memory context per group, with 8kB initial size, so
>     you're actually wasting ~99.999% of the memory
> (3) thanks to the dedicated memory contexts, the doubling is pretty
>     much pointless up until you cross the 8kB boundary

> IMNSHO these are the issues we really should fix - by lowering the
> initial element count (64->4) and using a single memory context.

The real issue here is that all those decisions are perfectly reasonable
if you expect that a large number of values will get aggregated --- and
even if you don't expect that, they're cheap insurance in simple cases.
It only gets to be a problem if you have a lot of concurrent executions
of array_agg, such as in a grouped-aggregate query.  You're essentially
arguing that in the grouped-aggregate case, it's better to optimize on
the assumption that only a very small number of values will get aggregated
(per hash table entry) --- which is possibly reasonable, but the argument
that it's okay to pessimize the behavior for other cases seems pretty
flimsy from here.

Actually, though, the patch as given outright breaks things for both the
grouped and ungrouped cases, because the aggregate no longer releases
memory when it's done.  That's going to result in memory bloat not
savings, in any situation where the aggregate is executed repeatedly.

I think a patch that stood a chance of getting committed would need to
detect whether the aggregate was being called in simple or grouped
contexts, and apply different behaviors in the two cases.  And you
can't just remove the sub-context without providing some substitute
cleanup mechanism.  Possibly you could keep the context but give it
some much-more-miserly allocation parameters in the grouped case.
        regards, tom lane

On 1.4.2014 19:08, Tom Lane wrote:
> Tomas Vondra <tv@fuzzy.cz> writes:
>> I've been thinking about it a bit more and maybe the doubling is not
>> that bad idea, after all.
> 
> It is not.  There's a reason why that's our standard behavior.
> 
>> The "current" array_agg however violates some of the assumptions
>> mentioned above, because it
>> (1) pre-allocates quite large number of items (64) at the beginning,
>>     resulting in ~98% of memory being "wasted" initially
>> (2) allocates one memory context per group, with 8kB initial size, so
>>     you're actually wasting ~99.999% of the memory
>> (3) thanks to the dedicated memory contexts, the doubling is pretty
>>     much pointless up until you cross the 8kB boundary
> 
>> IMNSHO these are the issues we really should fix - by lowering the
>> initial element count (64->4) and using a single memory context.
> 
> The real issue here is that all those decisions are perfectly
> reasonable if you expect that a large number of values will get
> aggregated --- and even if you don't expect that, they're cheap
> insurance in simple cases.

Yes, if you expect a large number of values it's perfectly valid. But
what if those assumptions are faulty? Is it OK to fail because of OOM
even for trivial queries breaking those assumptions?

I'd like to improve that and make this work without impacting the
queries that match the assumptions.

> It only gets to be a problem if you have a lot of concurrent
> executions of array_agg, such as in a grouped-aggregate query. You're
> essentially arguing that in the grouped-aggregate case, it's better
> to optimize on the assumption that only a very small number of values
> will get aggregated (per hash table entry) --- which is possibly
> reasonable, but the argument that it's okay to pessimize the behavior
> for other cases seems pretty flimsy from here.

I'm not saying it's okay to pessimize the behavior of other cases. I
admit decreasing the initial size from 64 to only 4 items may be too
aggressive - let's measure the difference and tweak the number
accordingly. Heck, even 64 items is way lower than the 8kB utilized by
each per-group memory context right now.


> Actually, though, the patch as given outright breaks things for both
> the grouped and ungrouped cases, because the aggregate no longer
> releases memory when it's done. That's going to result in memory
> bloat not savings, in any situation where the aggregate is executed
> repeatedly.

Really? Can you provide query for which the current and patched code
behave differently?

Looking at array_agg_finalfn (which is the final function for
array_agg), I see it does this:
   /*    * Make the result.  We cannot release the ArrayBuildState because    * sometimes aggregate final functions are
re-executed. Rather, it    * is nodeAgg.c's responsibility to reset the aggcontext when it's    * safe to do so.    */
result = makeMdArrayResult(state, 1, dims, lbs,                              CurrentMemoryContext,
       false);
 

i.e. it sets release=false. So I fail to see how the current code
behaves differently from the patch? If it wasn't releasing the memory
before, it's not releasing memory before.

In both cases the memory gets released when the aggcontext gets released
in nodeAgg.c (as explained by the comment in the code).

However, after looking at the code now, I think it's actually wrong to
remove the MemoryContextDelete from makeMdArrayResult(). It does not
make any difference to the array_agg (which sets release=false anyway),
but it makes difference to functions calling makeArrayResult() as that
uses release=true. That however is not called by aggregate functions,
but from regexp_split_to_array, xpath and subplans.

> I think a patch that stood a chance of getting committed would need to
> detect whether the aggregate was being called in simple or grouped
> contexts, and apply different behaviors in the two cases.  And you
> can't just remove the sub-context without providing some substitute
> cleanup mechanism.  Possibly you could keep the context but give it
> some much-more-miserly allocation parameters in the grouped case.

I don't think the patch removes any cleanup mechanism (see above), but
maybe I'm wrong.

Yes, tweaking the parameters depending on the aggregate - whether it's
simple or grouped, or maybe an estimate number of elements in a group -
seems like a good idea.

regards
Tomas

Tomas Vondra <tv@fuzzy.cz> writes:
> On 1.4.2014 19:08, Tom Lane wrote:
>> Actually, though, the patch as given outright breaks things for both
>> the grouped and ungrouped cases, because the aggregate no longer
>> releases memory when it's done. That's going to result in memory
>> bloat not savings, in any situation where the aggregate is executed
>> repeatedly.

> Looking at array_agg_finalfn (which is the final function for
> array_agg), I see it does this:

>     /*
>      * Make the result.  We cannot release the ArrayBuildState because
>      * sometimes aggregate final functions are re-executed.  Rather, it
>      * is nodeAgg.c's responsibility to reset the aggcontext when it's
>      * safe to do so.
>      */
>     result = makeMdArrayResult(state, 1, dims, lbs,
>                                CurrentMemoryContext,
>                                false);

> i.e. it sets release=false. So I fail to see how the current code
> behaves differently from the patch?

You're conveniently ignoring the callers that set release=true.
Reverse engineering a query that exhibits memory bloat is left
as an exercise for the reader (but in a quick look, I'll bet
ARRAY_SUBLINK subplans are one locus for problems).

It's possible that it'd work to use a subcontext only if release=true;
I've not dug through the code enough to convince myself of that.
        regards, tom lane

On 1.4.2014 20:56, Tom Lane wrote:
> Tomas Vondra <tv@fuzzy.cz> writes:
>> On 1.4.2014 19:08, Tom Lane wrote:
> You're conveniently ignoring the callers that set release=true.
> Reverse engineering a query that exhibits memory bloat is left
> as an exercise for the reader (but in a quick look, I'll bet
> ARRAY_SUBLINK subplans are one locus for problems).

No, I'm not. I explicitly mentioned those cases (although you're right I
concentrated mostly on cases with release=false, because of array_agg).

> It's possible that it'd work to use a subcontext only if
> release=true; I've not dug through the code enough to convince myself
> of that.

Maybe, though 'release' is not available in makeArrayResult() which is
where the memory context needs to be decided. So all the callers would
need to be modified to supply this parameter. But there only ~15 places
where makeArrayResult is called.

regards
Tomas

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

The v2 of the patch does this:

* adds 'subcontext' flag to initArrayResult* methods

  If it's 'true' then a separate context is created for the
  ArrayBuildState instance, otherwise it's built within the parent
  context.

  Currently, only the array_agg_* functions pass 'subcontext=false' so
  that the array_agg() aggregate does not create separate context for
  each group. All other callers use 'true' and thus keep using the
  original implementation (this includes ARRAY_SUBLINK subplans, and
  various other places building array incrementally).

* adds 'release' flag to makeArrayResult

  This is mostly to make it consistent with makeArrayResultArr and
  makeArrayResultAny. All current callers use '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).

* modifies all the places calling those functions

* decreases number of preallocated elements to 8

  The original value was 64 (512B), the current value is 64B. (Not
  counting the 'nulls' array). More about this later ...


Now, some performance measurements - attached is a simple SQL script
that executes a few GROUP BY queries with various numbers of groups and
group elements. I ran the tests with two dataset sizes:

small
=====
a) 1M groups, 1 item per group
b) 100k groups, 16 items per group
c) 100k groups, 64 items per group
d) 10k groups, 1024 items per group

large
=====
a) 10M groups, 1 item per group
b) 1M groups, 16 items per group
c) 1M groups, 64 items per group
d) 100k groups, 1024 items per group

So essentially the 'large' dataset uses 10x the number of groups. The
results (average from the 5 runs, in ms) look like this:

small
=====

test | master | patched | diff
-----|--------|---------|-------
   a |   1419 |     834 | -41%
   b |    595 |     498 | -16%
   c |   2061 |    1832 | -11%
   d |   2197 |    1957 | -11%

large
=====

test | master | patched | diff
-----|--------|---------|-------
   a |    OOM |    9144 |  n/a
   b |   7366 |    6257 | -15%
   c |  29899 |   22940 | -23%
   d |  35456 |   31347 | -12%

So it seems to give solid speedup across the whole test suite - I'm yet
to find a case where it's actually slower than what we have now. The
test cases (b) and (c) were actually created with this goal, because
both should be OK with the original array size (64 elements), but with
the new size it requires a few repalloc() calls. But even those are much
faster.

This is most likely thanks to removing the AllocSetContextCreate call
and sharing freelists across groups (although the test cases don't seem
extremely suitable for that, as all the groups grow in parallel).

I even tried to bump the initial array size back to 64 elements, but the
performance actually decreased a bit for some reason. I have no idea why
this happens ...

The test script is attached - tweak the 'size' variable for different
dataset sizes. The (insane) work_mem sizes are used to force a hash
aggregate - clearly I don't have 1TB of RAM.

regards
Tomas

On 15.12.2014 22:35, Jeff Janes wrote:
> On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra <tv@fuzzy.cz
> <mailto:tv@fuzzy.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.

regards
Tomas

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

Attached is v5 of the patch, fixing an error with releasing a shared
memory context (invalid flag values in a few calls).

kind regards
Tomas Vondra

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.

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

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Tomas Vondra <tv@fuzzy.cz> writes:
> 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.

FWIW, I quite dislike the terminology "shared memory context", because
it sounds too much like it means "a context in shared memory".  I see
that the patch itself doesn't use that phrase, which is good, but can
we come up with some other phrase for talking about it?
        regards, tom lane

On 12/21/14, 7:08 PM, Ali Akbar wrote:
>     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).
>
>
> Yes, with this API, we can backpatch this patch to 9.4 (or below) if we need it there.
>
> I think this API is a good compromise of old API and new API. Ideally if we can migrate all code to new API (all code
mustcall initArrayResult* before accumArrayResult*), we can remove parameter MemoryContext rcontext from
accumArrayResult.Currently, the code isn't using the rcontext for anything except for old API calls (in first call to
accumArrayResult).

Until we eliminate the API though, we should leave something in place that still uses the old one, to make certain we
don'taccidentally break it.
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Sun, 2014-12-21 at 13:00 -0500, Tom Lane wrote:
> Tomas Vondra <tv@fuzzy.cz> writes:
> > 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.
> 
> FWIW, I quite dislike the terminology "shared memory context", because
> it sounds too much like it means "a context in shared memory".  I see
> that the patch itself doesn't use that phrase, which is good, but can
> we come up with some other phrase for talking about it?
> 

"Common memory context"?

Regards,Jeff Davis

On Tue, 2014-12-16 at 00:27 +0100, Tomas Vondra wrote:
> > 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 jumping into this patch now. Do we think this is worth changing the
signature of functions in array.h, which might be used from a lot of
third-party code? We might want to provide new functions to avoid a
breaking change.

Regards,Jeff Davis

On Tue, 2014-04-01 at 13:08 -0400, Tom Lane wrote:
> I think a patch that stood a chance of getting committed would need to
> detect whether the aggregate was being called in simple or grouped
> contexts, and apply different behaviors in the two cases.

The simple context doesn't seem like a big problem even if we change
things as Tomas suggests:

"IMNSHO these are the issues we really should fix - by lowering the
initial element count (64->4) and using a single memory context."

In the simple context, there's only one context regardless, so the only
cost I see is from reducing the initial allocation from 64 to some lower
number. But if we're doubling each time, it won't take long to get
there; and because it's the simple context, we only need to do it once.

Regards,Jeff Davis

Hi,

On 8.1.2015 08:53, Ali Akbar wrote:
> In the CF, the status becomes "Needs Review". Let's continue our
> discussion of makeArrayResult* behavior if subcontext=false and
> release=true (more below):
> 2014-12-22 8:08 GMT+07:00 Ali Akbar <the.apaan@gmail.com
> <mailto:the.apaan@gmail.com>>:
>
>
>     With this API, i think we should make it clear if we call
>     initArrayResult with subcontext=false, we can't call
>     makeArrayResult, but we must use makeMdArrayResult directly.
>
>     Or better, we can modify makeArrayResult to release according to
>     astate->private_cxt:
>
>         @@ -4742,7 +4742,7 @@ makeArrayResult(ArrayBuildState *astate,
>             dims[0] = astate->nelems;
>             lbs[0] = 1;
>
>         -   return makeMdArrayResult(astate, ndims, dims, lbs, rcontext,
>         true);
>         +   return makeMdArrayResult(astate, ndims, dims, lbs, rcontext,
>         astate->private_cxt);

I've done this, so makeArrayResult() uses the private_cxt flag.

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

> As for the v6 patch:
> - the patch applies cleanly to master
> - make check is successfull
> - memory benefit is still there
> - performance benefit i think is negligible
>
> Reviewing the code, found this:
>
>     @@ -573,7 +578,22 @@ array_agg_array_transfn(PG_FUNCTION_ARGS)
>              elog(ERROR, "array_agg_array_transfn called in
>     non-aggregate context");
>          }
>
>     -    state = PG_ARGISNULL(0) ? NULL : (ArrayBuildStateArr *)
>     PG_GETARG_POINTER(0);
>     +
>     +    if (PG_ARGISNULL(0))
>     +    {
>     +        Oid            element_type = get_element_type(arg1_typeid);
>     +
>     +        if (!OidIsValid(element_type))
>     +            ereport(ERROR,
>     +                    (errcode(ERRCODE_DATATYPE_MISMATCH),
>     +                     errmsg("data type %s is not an array type",
>     +                            format_type_be(arg1_typeid))));
>
>
> digging more, it looks like those code required because
> accumArrayResultArr checks the element type:
>
>             /* First time through --- initialize */
>             Oid            element_type = get_element_type(array_type);
>
>             if (!OidIsValid(element_type))
>                 ereport(ERROR,
>                         (errcode(ERRCODE_DATATYPE_MISMATCH),
>                          errmsg("data type %s is not an array type",
>                                 format_type_be(array_type))));
>             astate = initArrayResultArr(array_type, element_type,
>     rcontext, true);
>
>
> I think it should be in initArrayResultArr, because it is an
> initialization-only check, shouldn't it?

Yes, seems reasonable. There are three places where initArrayResultArr
is called, and at two of them it looks just like the code you posted.

    Oid element_type = get_element_type(array_type);

    if (! OidIsValid(element_type)) { ... error ... }

    astate = initArrayResultArr(array_type, element_type, ...)

The last place is initArrayResultAny() is different because it uses the
element_type to decide whether to use initArrayResultArr or plain
initArrayResult. So the error-handling is missing here.

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

regards
Tomas Vondra

--
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12.1.2015 01:28, Ali Akbar wrote:
>
>     >     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.

Yes, I think it's worth mentioning.

>     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

Attached is v8 patch, with a few comments added:

1) before initArrayResult() - explaining when it's better to use a
   single memory context, and when it's more efficient to use a
   separate memory context for each array build state

2) before makeArrayResult() - explaining that it won't free memory
   when allocated in a single memory context (and that a pfree()
   has to be used if necessary)

3) before makeMdArrayResult() - explaining that it's illegal to use
   release=true unless using a subcontext

Otherwise the v8 patch is exactly the same as v7. Assuming the comments
make it sufficiently clear, I agree with marking this as 'ready for
committer'.

regards

--
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 14, 2015 at 1:52 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Attached is v8 patch, with a few comments added:
>
> 1) before initArrayResult() - explaining when it's better to use a
>    single memory context, and when it's more efficient to use a
>    separate memory context for each array build state
>
> 2) before makeArrayResult() - explaining that it won't free memory
>    when allocated in a single memory context (and that a pfree()
>    has to be used if necessary)
>
> 3) before makeMdArrayResult() - explaining that it's illegal to use
>    release=true unless using a subcontext
>

I understand there is history to this API, and we need to be
compatible, but the end result is awkward.

I'm wondering whether it would be better to just have a new set of
functions like accumArrayResultElem, etc., and only allow astate=NULL
in the original accumArrayResult(). That cure might be worse than the
disease though.

Regards,    Jeff Davis

On Sun, Dec 28, 2014 at 11:53 PM, Jeff Davis <pgsql@j-davis.com> wrote:
> On Tue, 2014-04-01 at 13:08 -0400, Tom Lane wrote:
>> I think a patch that stood a chance of getting committed would need to
>> detect whether the aggregate was being called in simple or grouped
>> contexts, and apply different behaviors in the two cases.
>
> The simple context doesn't seem like a big problem even if we change
> things as Tomas suggests:
>
> "IMNSHO these are the issues we really should fix - by lowering the
> initial element count (64->4) and using a single memory context."
>
> In the simple context, there's only one context regardless, so the only
> cost I see is from reducing the initial allocation from 64 to some lower
> number. But if we're doubling each time, it won't take long to get
> there; and because it's the simple context, we only need to do it once.

Tom (tgl),

Is my reasoning above acceptable?

The current patch, which I am evaluating for commit, does away with
per-group memory contexts (it uses a common context for all groups), and
reduces the initial array allocation from 64 to 8 (but preserves
doubling behavior).

Regards,Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> Tom (tgl),
> Is my reasoning above acceptable?

Uh, sorry, I've not been paying any attention to this thread for awhile.
What's the remaining questions at issue?
        regards, tom lane

Hi,

On 20.1.2015 12:23, Ali Akbar wrote:
> 2015-01-20 18:17 GMT+07:00 Ali Akbar <the.apaan@gmail.com
>
> Sorry, there is another comment of makeMdArrayResult, i suggest also
> changing it like this:
> @@ -4738,6 +4764,12 @@ makeArrayResult(ArrayBuildState *astate,
>   * beware: no check that specified dimensions match the number of values
>   * accumulated.
>   *
> + * beware: if the astate was not initialized within a separate memory
> + * context (i.e. using subcontext=true when calling initArrayResult),
> + * using release=true is illegal as it releases the whole context,
> + * and that may include other memory still used elsewhere (instead use
> + * release=false and release the memory with the parent context later)
> + *
>   *astate is working state (must not be NULL)
>   *rcontext is where to construct result

I think both comment fixes are appropriate. I'll wait a bit and then
post an updated version of the patch (unless it gets commited with the
comment fixes before that).

-- 
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Jeff Davis <pgsql@j-davis.com> writes:
>> Tom (tgl),
>> Is my reasoning above acceptable?
>
> Uh, sorry, I've not been paying any attention to this thread for awhile.
> What's the remaining questions at issue?

This patch is trying to improve the array_agg case where there are
many arrays being constructed simultaneously, such as in HashAgg. You
strongly suggested that a commitable patch would differentiate between
the grouped and ungrouped cases (or perhaps you meant differentiate
between HashAgg and sorted aggregation?). Tomas's current patch does
not do so; it does two main things:
  1. Uses a common memory context for arrays being constructed by
array_agg in any context (ungrouped, sorted agg, and HashAgg)  2. Reduces the initial array allocation to 8 elements
from64, but
 
preserves doubling behavior

I don't see either as a big problem, but perhaps there are some
downsides in some cases. I think a worst case would be a slowdown in
the sorted agg case where every group has 64 elements, so I'll try to
see if that's a real problem or not. If you saw a bigger problem,
please let me know; and if not, I'll proceed with the review.

There are also some other concerns I have about the API ugliness,
which I believe is the reason there's so much discussion about making
the comments better. The reason the API is ugly is for backwards
compatibility, so there's no perfect solution. Your opinion is welcome
here too, but I mainly need to see if your objection above has been
addressed.

Regards,   Jeff Davis

Hi,

On 20.1.2015 21:13, Jeff Davis wrote:
> On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Jeff Davis <pgsql@j-davis.com> writes:
>>> Tom (tgl),
>>> Is my reasoning above acceptable?
>>
>> Uh, sorry, I've not been paying any attention to this thread for awhile.
>> What's the remaining questions at issue?
> 
> This patch is trying to improve the array_agg case where there are 
> many arrays being constructed simultaneously, such as in HashAgg.
> You strongly suggested that a commitable patch would differentiate
> between the grouped and ungrouped cases (or perhaps you meant
> differentiate between HashAgg and sorted aggregation?). Tomas's
> current patch does not do so; it does two main things:

I don't think that's entirely true. The problem with the initial
(experimental) patch was that while it fixed aggregate queries, it
mostly ignored all the other callers, and either resulted in memory
corruption (unexpected pfree) or bloat (when not doint the pfree).

Tom's message where he points that out is here:
http://www.postgresql.org/message-id/20707.1396372100@sss.pgh.pa.us

The current patch does that distinction properly (IMHO), because it does
this distinction - all the callers using the underlying array functions
will use the original approach (with subcontexts), and only the
array_agg uses the new API and forces subcontext=false.

>    1. Uses a common memory context for arrays being constructed by
> array_agg in any context (ungrouped, sorted agg, and HashAgg)
>    2. Reduces the initial array allocation to 8 elements from 64, but
> preserves doubling behavior

Yes, that's true. I'd like to point out that while the current code uses
64 items, it also uses 8kB per-grop contexts. That's slightly overkill I
guess ...

> I don't see either as a big problem, but perhaps there are some 
> downsides in some cases. I think a worst case would be a slowdown in 
> the sorted agg case where every group has 64 elements, so I'll try
> to see if that's a real problem or not. If you saw a bigger problem, 
> please let me know; and if not, I'll proceed with the review.

FWIW I've done a fair amount of measurements and not noticed any
measurable difference (unless using a rather crazy testcase, IIRC).
Certainly the issues with excessive memory consumption (and swapping)
were much worse.

> There are also some other concerns I have about the API ugliness,
> which I believe is the reason there's so much discussion about making
> the comments better. The reason the API is ugly is for backwards
> compatibility, so there's no perfect solution. Your opinion is welcome
> here too, but I mainly need to see if your objection above has been
> addressed.

I generally agree that having two API 'facets' with different behavior
is slightly awkward and assymetric, but I wouldn't call that ugly. I
actually modified both APIs initially, but I think Ali is right that not
breaking the existing API (and keeping the original behavior in that
case) is better. We can break it any time we want in the future, but
it's impossible to "unbreak it" ;-)

regards

-- 
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, 2015-01-20 at 23:37 +0100, Tomas Vondra wrote:
> Tom's message where he points that out is here:
> http://www.postgresql.org/message-id/20707.1396372100@sss.pgh.pa.us

That message also says:

"I think a patch that stood a chance of getting committed would need to
detect whether the aggregate was being called in simple or grouped
contexts, and apply different behaviors in the two cases."

I take that as an objection to any patch which does not distinguish
between the grouped and ungrouped aggregate cases, which includes your
patch.

I don't agree with that objection (or perhaps I don't understand it);
but given the strong words above, I need to get some kind of response
before I can consider committing your patch.

> I generally agree that having two API 'facets' with different behavior
> is slightly awkward and assymetric, but I wouldn't call that ugly.

Right, your words are more precise (and polite). My apologies.

> I
> actually modified both APIs initially, but I think Ali is right that not
> breaking the existing API (and keeping the original behavior in that
> case) is better. We can break it any time we want in the future, but
> it's impossible to "unbreak it" ;-)

We can't break the old API, and I'm not suggesting that we do. I was
hoping to find some alternative.

Regards,Jeff Davis

Hi,

On 21.1.2015 09:01, Jeff Davis wrote:
> On Tue, 2015-01-20 at 23:37 +0100, Tomas Vondra wrote:
>> Tom's message where he points that out is here:
>> http://www.postgresql.org/message-id/20707.1396372100@sss.pgh.pa.us
> 
> That message also says:
> 
> "I think a patch that stood a chance of getting committed would need
> to detect whether the aggregate was being called in simple or
> grouped contexts, and apply different behaviors in the two cases."
> 
> I take that as an objection to any patch which does not distinguish 
> between the grouped and ungrouped aggregate cases, which includes
> your patch.

I don't think 'simple context' in this case means 'aggregate without a
group by clause'.

The way I understood it back in April 2014 was that while the patch
worked fine with grouped cases (i.e. in nodeAgg.c or such), the
underlying function are used elsewhere in a simple context (e.g. in an
xpath() or so) - and in this case it was broken. And that was a correct
point, and was fixed by the recent patches.

But maybe I'm missing something?

> I don't agree with that objection (or perhaps I don't understand
> it); but given the strong words above, I need to get some kind of
> response before I can consider committing your patch.
> 
>> I generally agree that having two API 'facets' with different behavior
>> is slightly awkward and assymetric, but I wouldn't call that ugly.
> 
> Right, your words are more precise (and polite). My apologies.

Ummmm ... I wasn't suggesting calling the resulting API 'ugly' is
impolite or so. It was meant just as a comment that the aesthetics of
the API is quite subjective matter. No apology needed.

>> I actually modified both APIs initially, but I think Ali is right 
>> that not breaking the existing API (and keeping the original
>> behavior in that case) is better. We can break it any time we want
>> in the future, but it's impossible to "unbreak it" ;-)
> 
> We can't break the old API, and I'm not suggesting that we do. I was
> hoping to find some alternative.

Why can't we? I'm not saying we should in this cases, but there are
cases when breaking the API is the right thing to do (e.g. when the
behavior changes radically, and needs to be noticed by the users).


-- 
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Jeff Davis <pgsql@j-davis.com> writes:
> On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Uh, sorry, I've not been paying any attention to this thread for awhile.
>> What's the remaining questions at issue?

> This patch is trying to improve the array_agg case where there are
> many arrays being constructed simultaneously, such as in HashAgg. You
> strongly suggested that a commitable patch would differentiate between
> the grouped and ungrouped cases (or perhaps you meant differentiate
> between HashAgg and sorted aggregation?). Tomas's current patch does
> not do so; it does two main things:
>    1. Uses a common memory context for arrays being constructed by
> array_agg in any context (ungrouped, sorted agg, and HashAgg)
>    2. Reduces the initial array allocation to 8 elements from 64, but
> preserves doubling behavior

Sorry for slow response on this.  I took a look at the v8 patch (that's
still latest no?).  I see that it only gets rid of the separate context
for the two callers array_agg_transfn and array_agg_array_transfn, for
which no memory release can happen anyway until the parent context is
reset (cf comments in corresponding finalfns).  So that's fine --- it is
not making any bloat case any worse, at least.

I'm not sure about whether reducing the initial Datum array size
across-the-board is a good thing or not.  One obvious hack to avoid
unexpected side effects on other callers would be
astate->alen = (subcontext ? 64 : 8);

The larger initial size is basically free when subcontext is true, anyway,
considering it will be coming out of an 8K initial subcontext allocation.

This still leaves us wondering whether the smaller initial size will
hurt array_agg itself, but that's at least capable of being tested
with a reasonably small number of test cases.

I strongly object to removing initArrayResultArr's element_type argument.
That's got nothing to do with the stated purpose of the patch, and it
forces a non-too-cheap catalog lookup to be done inside
initArrayResultArr.  It's true that some of the current call sites are
just doing the same lookup themselves anyway, but we should not foreclose
the possibility that the caller has the data already (as some others do)
or is able to cache it across multiple uses.  A possible compromise is
to add the convention that callers can pass InvalidOid if they want
initArrayResultArr to do the lookup.  (In any case, the function's header
comment needs adjustment if the API spec changes.)

Other than that, I think the API changes here are OK.  Adding a new
argument to existing functions is something we do all the time, and it's
clear how to modify any callers to get the same behavior as before.
We could perhaps clean things up with a more invasive redefinition, but
I doubt it's worth inflicting more pain on third party callers.

Another thing I'd change is this:
+    /* 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);

Why not this way:
    /* Clean up all the junk */    if (release)
+    {
+        Assert(astate->private_cxt);        MemoryContextDelete(astate->mcontext);
+    }

Seems a lot more understandable, and less code too.

I concur with the concerns that the comments could do with more work,
but haven't attempted to improve them myself.
        regards, tom lane

On 28.1.2015 21:28, Tom Lane wrote:
> Jeff Davis <pgsql@j-davis.com> writes:
>> On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Uh, sorry, I've not been paying any attention to this thread for awhile.
>>> What's the remaining questions at issue?
> 
>> This patch is trying to improve the array_agg case where there are
>> many arrays being constructed simultaneously, such as in HashAgg. You
>> strongly suggested that a commitable patch would differentiate between
>> the grouped and ungrouped cases (or perhaps you meant differentiate
>> between HashAgg and sorted aggregation?). Tomas's current patch does
>> not do so; it does two main things:
>>    1. Uses a common memory context for arrays being constructed by
>> array_agg in any context (ungrouped, sorted agg, and HashAgg)
>>    2. Reduces the initial array allocation to 8 elements from 64, but
>> preserves doubling behavior
> 
> Sorry for slow response on this. I took a look at the v8 patch
> (that's still latest no?). I see that it only gets rid of the

Yes, v8 is the latest version I submitted.

> separate context for the two callers array_agg_transfn and
> array_agg_array_transfn, for which no memory release can happen
> anyway until the parent context is reset (cf comments in
> corresponding finalfns). So that's fine --- it is not making any
> bloat case any worse, at least.
> 
> I'm not sure about whether reducing the initial Datum array size 
> across-the-board is a good thing or not. One obvious hack to avoid 
> unexpected side effects on other callers would be
> 
>     astate->alen = (subcontext ? 64 : 8);
> 
> The larger initial size is basically free when subcontext is true,
> anyway, considering it will be coming out of an 8K initial subcontext
> allocation.

Seems like a good idea. If we're using 8kB contexts, we can preallocate
64 elements right away.

But maybe we could decrease the 8kB context size to 1kB? For 64 elements
is just 512B + 64B for the arrays, and a bit of space for the
ArrayBuildState. So maybe ~600B in total.

Of course, this does not include space for pass-by-ref values.

Anyway, I have no  problem with this - I mostly care just about the
array_agg() case. All the other places may adopt the  'common context'
approach to get the benefit.

> This still leaves us wondering whether the smaller initial size will
>  hurt array_agg itself, but that's at least capable of being tested 
> with a reasonably small number of test cases.

I plan to do more testing to confirm this - my initial testing seemed to
confirm this, but I'll repeat that with the current patch.

> I strongly object to removing initArrayResultArr's element_type
> argument. That's got nothing to do with the stated purpose of the
> patch, and it forces a non-too-cheap catalog lookup to be done
> inside initArrayResultArr.  It's true that some of the current call
> sites are just doing the same lookup themselves anyway, but we should
> not foreclose the possibility that the caller has the data already
> (as some others do) or is able to cache it across multiple uses.  A
> possible compromise is to add the convention that callers can pass
> InvalidOid if they want initArrayResultArr to do the lookup.  (In any
> case, the function's header comment needs adjustment if the API spec
> changes.)

Fair point, and the InvalidOid approach seems sane to me.

> 
> Other than that, I think the API changes here are OK. Adding a new 
> argument to existing functions is something we do all the time, and
> it's clear how to modify any callers to get the same behavior as
> before. We could perhaps clean things up with a more invasive
> redefinition, but I doubt it's worth inflicting more pain on third
> party callers.
> 
> Another thing I'd change is this:
>  
> +    /* 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);
> 
> Why not this way:
> 
>      /* Clean up all the junk */
>      if (release)
> +    {
> +        Assert(astate->private_cxt);
>          MemoryContextDelete(astate->mcontext);
> +    }
> 
> Seems a lot more understandable, and less code too.

Yeah, I agree it's easier to understand.

> I concur with the concerns that the comments could do with more
> work, but haven't attempted to improve them myself.

There were a few comments about this, after the v8 patch, with
recommended comment changes.

regards

-- 
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

attached is v9 of the patch, modified along the lines of Tom's comments:

1) uses alen=64 for cases with private context, 8 otherwise

2) reverts removal of element_type from initArrayResultArr()

   When element_type=InvalidOid is passed to initArrayResultArr, it
   performs lookup using get_element_type(), otherwise reuses the value
   it receives from the caller.

3) moves the assert into the 'if (release)' branch

4) includes the comments proposed by Ali Akbar in his reviews

   Warnings at makeArrayResult/makeMdArrayResult about freeing memory
   with private subcontexts.


Regarding the performance impact of decreasing the size of the
preallocated array from 64 to just 8 elements, I tried this.

  CREATE TABLE test AS
  SELECT mod(i,100000) a, i FROM generate_series(1,64*100000) s(i);

  SELECT a, array_agg(i) AS x FRM test GROUP BY 1;

or actually (to minimize transfer overhead):

  SELECT COUNT(x) FROM (
     SELECT a, array_agg(i) AS x FRM test GROUP BY 1
  ) foo;

with work_mem=2GB (so that it really uses HashAggregate). The dataset is
constructed to have exactly 64 items per group, thus exploiting the
difference between alen=8 and alen=64.

With alen=8 I get these timings:

Time: 1892,681 ms
Time: 1879,046 ms
Time: 1892,626 ms
Time: 1892,155 ms
Time: 1880,282 ms
Time: 1868,344 ms
Time: 1873,294 ms

and with alen=64:

Time: 1888,244 ms
Time: 1882,991 ms
Time: 1885,157 ms
Time: 1868,935 ms
Time: 1878,053 ms
Time: 1894,871 ms
Time: 1871,571 ms

That's 1880 vs 1882 on average, so pretty much no difference. Would be
nice if someone else could try this on their machine(s).

regards

--
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/28/15 4:25 PM, Tomas Vondra wrote:
> + * It's possible to choose whether to create a separate memory context for the
> + * array builde state, or whether to allocate it directly within rcontext

Typo.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Thu, Jan 29, 2015 at 7:25 AM, Tomas
Vondra<spandir="ltr"><a href="mailto:tomas.vondra@2ndquadrant.com" target="_blank"></a></span> wrote:<br /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">attached is v9 of the patch,
modifiedalong the lines of Tom's comments:<br /></blockquote></div><br /></div><div class="gmail_extra">Moved this
patchto next CF, hopefully it will get more attention, and a reviewer.<br />-- <br /><div
class="gmail_signature">Michael<br/></div></div></div> 

On Wed, 2015-01-28 at 23:25 +0100, Tomas Vondra wrote:
> 3) moves the assert into the 'if (release)' branch

You missed one, but I got it.

> 4) includes the comments proposed by Ali Akbar in his reviews
>
>    Warnings at makeArrayResult/makeMdArrayResult about freeing memory
>    with private subcontexts.

I also edited the comments substantially.

> Regarding the performance impact of decreasing the size of the
> preallocated array from 64 to just 8 elements, I tried this.
>
>   CREATE TABLE test AS
>   SELECT mod(i,100000) a, i FROM generate_series(1,64*100000) s(i);
>
>   SELECT a, array_agg(i) AS x FRM test GROUP BY 1;
>
> or actually (to minimize transfer overhead):
>
>   SELECT COUNT(x) FROM (
>      SELECT a, array_agg(i) AS x FRM test GROUP BY 1
>   ) foo;

That's actually a bogus test -- array_agg is never executed. See the
test script I used (attached). Many small groups has a significant
improvement with your patch (median 167ms unpatched, 125ms patched), and
the one large group is not measurably different (median 58ms for both).

The test script uses a small dataset of 100K tuples, because 1M tuples
will run out of memory for small groups without the patch.

Committed.

Regards,
    Jeff Davis

On 22.2.2015 02:38, Jeff Davis wrote:
> On Wed, 2015-01-28 at 23:25 +0100, Tomas Vondra wrote:
>> 3) moves the assert into the 'if (release)' branch
> 
> You missed one, but I got it.

Oh, thanks!

> 
>> 4) includes the comments proposed by Ali Akbar in his reviews
>>
>>    Warnings at makeArrayResult/makeMdArrayResult about freeing memory
>>    with private subcontexts.
> 
> I also edited the comments substantially.

Thanks.

>> Regarding the performance impact of decreasing the size of the
>> preallocated array from 64 to just 8 elements, I tried this.
>>
>>   CREATE TABLE test AS
>>   SELECT mod(i,100000) a, i FROM generate_series(1,64*100000) s(i);
>>
>>   SELECT a, array_agg(i) AS x FRM test GROUP BY 1;
>>
>> or actually (to minimize transfer overhead):
>>
>>   SELECT COUNT(x) FROM (
>>      SELECT a, array_agg(i) AS x FRM test GROUP BY 1
>>   ) foo;
> 
> That's actually a bogus test -- array_agg is never executed.

Really? How could that happen when the result of array_agg() is passed
to the COUNT()? Also, how could that allocate huge amounts of memory and
get killed by OOM, which happens easily with this query?

> See the test script I used (attached). Many small groups has a
> significant improvement with your patch (median 167ms unpatched,
> 125ms patched), and the one large group is not measurably different
> (median 58ms for both).

Yup, that's expected. With a lot of small groups the improvement may
easily be much higher, but this is a conservative test.

> 
> The test script uses a small dataset of 100K tuples, because 1M
> tuples will run out of memory for small groups without the patch.
> 
> Committed.

Cool. Thanks once more.


regards

-- 
Tomas Vondra                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, 2015-02-22 at 03:14 +0100, Tomas Vondra wrote:
> >>   SELECT COUNT(x) FROM (
> >>      SELECT a, array_agg(i) AS x FRM test GROUP BY 1
> >>   ) foo;
> > 
> > That's actually a bogus test -- array_agg is never executed.
> 
> Really? How could that happen when the result of array_agg() is passed
> to the COUNT()? Also, how could that allocate huge amounts of memory and
> get killed by OOM, which happens easily with this query?

Oops, I misread that as "COUNT(*)". Count(x) will force array_agg() to
be executed.

Regards,Jeff Davis