Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 pas

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 makeArrayRes

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 su

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Thu, Jan 29, 2015 at 7:25 AM, Tomas Vondra wrote: > attached is v9 of the patch, modified along the lines of Tom's comments: > Moved this patch to next CF, hopefully it will get more attention, and a reviewer. -- Michael

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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:/

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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_

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On 28.1.2015 21:28, Tom Lane wrote: > Jeff Davis writes: >> On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane 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 the

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Jeff Davis writes: > On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane 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 simul

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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.1396372...@sss.pgh.pa.us > > That message also says: > > "I think a patch that stood a chance of get

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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.1396372...@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 aggrega

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Hi, On 20.1.2015 21:13, Jeff Davis wrote: > On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane wrote: >> Jeff Davis 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? > >

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane wrote: > Jeff Davis 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 wher

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Hi, On 20.1.2015 12:23, Ali Akbar wrote: > 2015-01-20 18:17 GMT+07:00 Ali Akbar > 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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Jeff Davis 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2015-01-20 18:17 GMT+07:00 Ali Akbar : > 2015-01-20 14:22 GMT+07:00 Jeff Davis : > >> 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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2015-01-20 14:22 GMT+07:00 Jeff Davis : > 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). Jeff & Tomas, spotted

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Sun, Dec 28, 2014 at 11:53 PM, Jeff Davis 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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Wed, Jan 14, 2015 at 1:52 PM, Tomas Vondra 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 >

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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

Re: [HACKERS] 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->m

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 >: > > > With this API,

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 : > > With this API, i think we should make it clear if we call initArrayResult > with subcontext=false, w

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2014-12-29 14:38 GMT+07:00 Jeff Davis : > 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. > V6 patch from Tomas

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 pr

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 'initArra

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Sun, 2014-12-21 at 13:00 -0500, Tom Lane wrote: > Tomas Vondra 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 > >

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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.

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Tomas Vondra 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 so

Re: [HACKERS] 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 ar

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index d9faf20..9c97755 100644 --- a/src/backend/executor/nodeSubp

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 >: > > > > 2014-12-16 10:47 GMT+07:00 Ali Akbar

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2014-12-16 11:01 GMT+07:00 Ali Akbar : > > > > 2014-12-16 10:47 GMT+07:00 Ali Akbar : >> >> >> 2014-12-16 6:27 GMT+07:00 Tomas Vondra : >> Just fast-viewing the patch. >> >> The patch is not implementing the checking for not creating new context >> in initArrayResultArr. I think we should implement

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2014-12-16 10:47 GMT+07:00 Ali Akbar : > > > 2014-12-16 6:27 GMT+07:00 Tomas Vondra : >> >> On 15.12.2014 22:35, Jeff Janes wrote: >> > On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra > > > wrote: >> > >> > Hi, >> > >> > Attached is v2 of the patch lowering array_agg mem

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

2014-12-16 6:27 GMT+07:00 Tomas Vondra : > > On 15.12.2014 22:35, Jeff Janes wrote: > > On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra > > wrote: > > > > Hi, > > > > Attached is v2 of the patch lowering array_agg memory requirements. > > Hopefully it addresses the

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On 15.12.2014 22:35, Jeff Janes wrote: > On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra > 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 h

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra 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 --wi

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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 't

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On 1.4.2014 20:56, Tom Lane wrote: > Tomas Vondra 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_

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Tomas Vondra 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 sit

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On 1.4.2014 19:08, Tom Lane wrote: > Tomas Vondra 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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

Tomas Vondra 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-alloca

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

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.

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On 31.3.2014 21:04, Robert Haas wrote: > On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra 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

Re: [HACKERS] PATCH: decreasing memory needlessly consumed by array_agg

On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra 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->n

[HACKERS] 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.nfa2rvc...@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 fo