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
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
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
On Thu, Jan 29, 2015 at 7:25 AM, Tomas Vondra tomas.von...@2ndquadrant.com
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
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!
Jeff Davis pg...@j-davis.com writes:
On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane t...@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
On 28.1.2015 21:28, Tom Lane wrote:
Jeff Davis pg...@j-davis.com writes:
On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane t...@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
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
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 getting
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 aggregate
2015-01-20 18:17 GMT+07:00 Ali Akbar the.ap...@gmail.com:
2015-01-20 14:22 GMT+07:00 Jeff Davis pg...@j-davis.com:
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
2015-01-20 14:22 GMT+07:00 Jeff Davis pg...@j-davis.com:
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
Jeff Davis pg...@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
--
Sent via pgsql-hackers mailing list
Hi,
On 20.1.2015 12:23, Ali Akbar wrote:
2015-01-20 18:17 GMT+07:00 Ali Akbar the.ap...@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
On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Jeff Davis pg...@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
Hi,
On 20.1.2015 21:13, Jeff Davis wrote:
On Tue, Jan 20, 2015 at 6:44 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Jeff Davis pg...@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
On Wed, Jan 14, 2015 at 1:52 PM, Tomas Vondra
tomas.von...@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
On Sun, Dec 28, 2014 at 11:53 PM, Jeff Davis pg...@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
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
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);
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.ap...@gmail.com
mailto:the.ap...@gmail.com:
With
2014-12-29 14:38 GMT+07:00 Jeff Davis pg...@j-davis.com:
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
On Sun, 2014-12-21 at 13:00 -0500, Tom Lane wrote:
Tomas Vondra t...@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
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
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
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
Tomas Vondra t...@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
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
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
2014-12-16 11:01 GMT+07:00 Ali Akbar the.ap...@gmail.com:
2014-12-16 10:47 GMT+07:00 Ali Akbar the.ap...@gmail.com:
2014-12-16 6:27 GMT+07:00 Tomas Vondra t...@fuzzy.cz:
Just fast-viewing the patch.
The patch is not implementing the checking for not creating new context
in
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.ap...@gmail.com
mailto:the.ap...@gmail.com:
2014-12-16 10:47 GMT+07:00 Ali Akbar the.ap...@gmail.com
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
---
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?
@@
2014-12-16 6:27 GMT+07:00 Tomas Vondra t...@fuzzy.cz:
On 15.12.2014 22:35, Jeff Janes wrote:
On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra t...@fuzzy.cz
mailto:t...@fuzzy.cz wrote:
Hi,
Attached is v2 of the patch lowering array_agg memory requirements.
Hopefully it
2014-12-16 10:47 GMT+07:00 Ali Akbar the.ap...@gmail.com:
2014-12-16 6:27 GMT+07:00 Tomas Vondra t...@fuzzy.cz:
On 15.12.2014 22:35, Jeff Janes wrote:
On Sat, Nov 29, 2014 at 8:57 AM, Tomas Vondra t...@fuzzy.cz
mailto:t...@fuzzy.cz wrote:
Hi,
Attached is v2 of the patch
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
On 31.3.2014 21:04, Robert Haas wrote:
On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra t...@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
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
Tomas Vondra t...@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)
On 1.4.2014 19:08, Tom Lane wrote:
Tomas Vondra t...@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
Tomas Vondra t...@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
On 1.4.2014 20:56, Tom Lane wrote:
Tomas Vondra t...@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
On Thu, Mar 27, 2014 at 10:00 PM, Tomas Vondra t...@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
43 matches
Mail list logo
