On 22.2.2015 09:14, Jeff Davis wrote:
On Wed, 2015-01-07 at 20:07 +0100, Tomas Vondra wrote:
So I started digging in the code and I noticed this:
hash_mem = MemoryContextMemAllocated(aggstate-hashcontext, true);
which is IMHO obviously wrong, because that accounts only for the
On Wed, 2015-01-07 at 20:07 +0100, Tomas Vondra wrote:
So I started digging in the code and I noticed this:
hash_mem = MemoryContextMemAllocated(aggstate-hashcontext, true);
which is IMHO obviously wrong, because that accounts only for the
hashtable itself. It might be correct for
Hi,
On 23.12.2014 10:16, Jeff Davis wrote:
It seems that these two patches are being reviewed together. Should
I just combine them into one? My understanding was that some wanted
to review the memory accounting patch separately.
On Sun, 2014-12-21 at 20:19 +0100, Tomas Vondra wrote:
That's
On Sun, Dec 28, 2014 at 12:45 PM, Peter Geoghegan p...@heroku.com wrote:
On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis pg...@j-davis.com wrote:
Do others have similar numbers? I'm quite surprised at how little
work_mem seems to matter for these plans (HashJoin might be a different
story
On 12/28/14, 2:45 PM, Peter Geoghegan wrote:
On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis pg...@j-davis.com wrote:
Do others have similar numbers? I'm quite surprised at how little
work_mem seems to matter for these plans (HashJoin might be a different
story though). I feel like I made a
On Tue, 2014-12-23 at 01:16 -0800, Jeff Davis wrote:
New patch attached (rebased, as well).
I also see your other message about adding regression testing. I'm
hesitant to slow down the tests for everyone to run through this code
path though. Should I add regression tests, and then remove
On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis pg...@j-davis.com wrote:
Do others have similar numbers? I'm quite surprised at how little
work_mem seems to matter for these plans (HashJoin might be a different
story though). I feel like I made a mistake -- can someone please do a
sanity check on
On Sun, 2014-12-28 at 12:37 -0800, Jeff Davis wrote:
I feel like I made a mistake -- can someone please do a
sanity check on my numbers?
I forgot to randomize the inputs, which doesn't matter much for hashagg
but does matter for sort. New data script attached. The results are even
*better* for
It seems that these two patches are being reviewed together. Should I
just combine them into one? My understanding was that some wanted to
review the memory accounting patch separately.
On Sun, 2014-12-21 at 20:19 +0100, Tomas Vondra wrote:
That's the only conflict, and after fixing it it
On 23.12.2014 10:16, Jeff Davis wrote:
It seems that these two patches are being reviewed together. Should
I just combine them into one? My understanding was that some wanted
to review the memory accounting patch separately.
I think we should keep the patches separate. Applying two patches is
On 2.12.2014 06:14, Jeff Davis wrote:
On Sun, 2014-11-30 at 17:49 -0800, Peter Geoghegan wrote:
On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis pg...@j-davis.com wrote:
I can also just move isReset there, and keep mem_allocated as a uint64.
That way, if I find later that I want to track the
On 21.12.2014 20:19, Tomas Vondra wrote:
However, I got a segfault on the very first query I tried :-(
create table test_hash_agg as select i AS a, i AS b, i AS c, i AS d
from generate_series(1,1000) s(i);
analyze test_hash_agg;
select a, count(*) from test_hash_agg where
On Tue, Dec 2, 2014 at 2:14 PM, Jeff Davis pg...@j-davis.com wrote:
On Sun, 2014-11-30 at 17:49 -0800, Peter Geoghegan wrote:
On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis pg...@j-davis.com wrote:
I can also just move isReset there, and keep mem_allocated as a uint64.
That way, if I find
On Sun, 2014-11-30 at 17:49 -0800, Peter Geoghegan wrote:
On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis pg...@j-davis.com wrote:
I can also just move isReset there, and keep mem_allocated as a uint64.
That way, if I find later that I want to track the aggregated value for
the child contexts
On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis pg...@j-davis.com wrote:
I can also just move isReset there, and keep mem_allocated as a uint64.
That way, if I find later that I want to track the aggregated value for
the child contexts as well, I can split it into two uint32s. I'll hold
off any
On 21.11.2014 00:03, Andres Freund wrote:
On 2014-11-17 21:03:07 +0100, Tomas Vondra wrote:
On 17.11.2014 19:46, Andres Freund wrote:
The MemoryContextData struct is embedded into AllocSetContext.
Oh, right. That makes is slightly more complicated, though, because
AllocSetContext adds 6 x
On 2014-11-17 21:03:07 +0100, Tomas Vondra wrote:
On 17.11.2014 19:46, Andres Freund wrote:
On 2014-11-17 19:42:25 +0100, Tomas Vondra wrote:
On 17.11.2014 18:04, Andres Freund wrote:
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
---
On 11/17/14, 10:50 AM, Tomas Vondra wrote:
Either of these restrictions would prevent a situation where a context
has to update accounting for two parent contexts. That'd allow updating
a single place (because each context has a single parent context with
tracking requested).
Another option
On 17 November 2014 07:31, Jeff Davis pg...@j-davis.com wrote:
On Sat, 2014-11-15 at 21:36 +, Simon Riggs wrote:
Do I understand correctly that we are trying to account for exact
memory usage at palloc/pfree time? Why??
Not palloc chunks, only tracking at the level of allocated blocks
On 17 November 2014 07:31, Jeff Davis pg...@j-davis.com wrote:
To calculate the total memory used, I included a function
MemoryContextMemAllocated() that walks the memory context and its
children recursively.
If we assume that we want the results accurate to within 5%, then we
can work out a
On 15.11.2014 22:36, Simon Riggs wrote:
On 16 October 2014 02:26, Jeff Davis pg...@j-davis.com wrote:
The inheritance is awkward anyway, though. If you create a tracked
context as a child of an already-tracked context, allocations in
the newer one won't count against the original. I don't
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
--- b/src/include/nodes/memnodes.h
***
*** 60,65 typedef struct MemoryContextData
--- 60,66
MemoryContext nextchild;/* next child of same parent */
char
On 17.11.2014 08:31, Jeff Davis wrote:
On Sat, 2014-11-15 at 21:36 +, Simon Riggs wrote:
Do I understand correctly that we are trying to account for exact
memory usage at palloc/pfree time? Why??
Not palloc chunks, only tracking at the level of allocated blocks
(that we allocate with
On 17.11.2014 18:04, Andres Freund wrote:
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
--- b/src/include/nodes/memnodes.h
***
*** 60,65 typedef struct MemoryContextData
--- 60,66
MemoryContext nextchild;/* next
On 2014-11-17 19:42:25 +0100, Tomas Vondra wrote:
On 17.11.2014 18:04, Andres Freund wrote:
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
--- b/src/include/nodes/memnodes.h
***
*** 60,65 typedef struct MemoryContextData
On 17.11.2014 19:46, Andres Freund wrote:
On 2014-11-17 19:42:25 +0100, Tomas Vondra wrote:
On 17.11.2014 18:04, Andres Freund wrote:
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
--- b/src/include/nodes/memnodes.h
***
*** 60,65
On Mon, 2014-11-17 at 18:04 +0100, Andres Freund wrote:
That's quite possibly one culprit for the slowdown. Right now one
AllocSetContext struct fits precisely into three cachelines. After your
change it doesn't anymore.
Thank you! I made the same mistake as Tomas: I saw that
MemoryContextData
On 16 October 2014 02:26, Jeff Davis pg...@j-davis.com wrote:
The inheritance is awkward anyway, though. If you create a tracked
context as a child of an already-tracked context, allocations in the
newer one won't count against the original. I don't see a way around
that without introducing
Hi,
On 16.10.2014 03:26, Jeff Davis wrote:
On Fri, 2014-08-29 at 10:12 -0400, Tom Lane wrote:
What about removing the callback per se and just keeping the
argument, as it were. That is, a MemoryContext has a field of type
size_t * that is normally NULL, but if set to non-NULL, then we
On Fri, 2014-08-29 at 10:12 -0400, Tom Lane wrote:
What about removing the callback per se and just keeping the argument,
as it were. That is, a MemoryContext has a field of type size_t *
that is normally NULL, but if set to non-NULL, then we increment the
pointed-to value for pallocs and
On Fri, 2014-08-22 at 12:34 -0400, Robert Haas wrote:
Still doesn't look good here. On the same PPC64 machine I've been
using for testing:
I have a new approach to the patch which is to call a callback at each
block allocation and child contexts inherit the callback from their
parents. The
Jeff Davis pg...@j-davis.com writes:
I have a new approach to the patch which is to call a callback at each
block allocation and child contexts inherit the callback from their
parents. The callback could be defined to simply dereference and
increment its argument, which would mean block
On 29.8.2014 16:12, Tom Lane wrote:
Jeff Davis pg...@j-davis.com writes:
I have a new approach to the patch which is to call a callback at each
block allocation and child contexts inherit the callback from their
parents. The callback could be defined to simply dereference and
increment its
On Wed, Aug 20, 2014 at 2:11 AM, Jeff Davis pg...@j-davis.com wrote:
I attached a patch that uses two uint32 fields so that it doesn't
increase the size of MemoryContextData, and it tracks memory usage for
all contexts. I was unable to detect any performance regression vs.
master, but on my
On 20.8.2014 08:11, Jeff Davis wrote:
On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
The use-case for this is tracking a chosen subtree of contexts - e.g.
aggcontext and below, so I'd expect the tracked subtrees to be relatively
shallow. Am I right?
Right.
My fear is that by
On Fri, Aug 22, 2014 at 2:13 PM, Tomas Vondra t...@fuzzy.cz wrote:
I don't think we really need to abandon the 'tracked' flag (or that we
should). I think it was useful, and removing it might be one of the
reasons why Robert now sees worse impact than before.
The version that introduced that
On 20.8.2014 08:11, Jeff Davis wrote:
On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
It would be easier to resolve the performance concern if I could
reliably get the results Robert is getting. I think I was able to
reproduce the regression with the old patch, but the results were
On Tue, 2014-08-19 at 12:54 +0200, Tomas Vondra wrote:
The use-case for this is tracking a chosen subtree of contexts - e.g.
aggcontext and below, so I'd expect the tracked subtrees to be relatively
shallow. Am I right?
Right.
My fear is that by removing the inheritance bit, we'll hurt cases
On Thu, 2014-08-14 at 12:52 -0400, Robert Haas wrote:
It appears to me that the performance characteristics for this version
are not significantly different from version 1. I have not looked at
the code.
While trying to reproduce your results, I noticed what might be around a
1% regression
On Sat, 2014-08-16 at 23:09 +0200, Tomas Vondra wrote:
But maybe the inheritance really is not necessary - maybe it would be
enough to track this per-context, and then just walk through the
contexts and collect this. Because my observation is that most of the
time is actually spent in walking
On 19 Srpen 2014, 10:26, Jeff Davis wrote:
On Sat, 2014-08-16 at 23:09 +0200, Tomas Vondra wrote:
But maybe the inheritance really is not necessary - maybe it would be
enough to track this per-context, and then just walk through the
contexts and collect this. Because my observation is that
On 10.8.2014 22:50, Jeff Davis wrote:
On Fri, 2014-08-08 at 01:16 -0700, Jeff Davis wrote:
Either way, it's better to be conservative. Attached is a version
of the patch with opt-in memory usage tracking. Child contexts
inherit the setting from their parent.
There was a problem with the
Tomas Vondra t...@fuzzy.cz writes:
I believe this should check parent-track_mem, just like
update_allocation, because this way it walks all the parent context up
to the TopMemoryContext.
TBH, I don't think this opt-in tracking business has been thought
through even a little bit; for example,
On 16.8.2014 20:00, Tom Lane wrote:
Tomas Vondra t...@fuzzy.cz writes:
I believe this should check parent-track_mem, just like
update_allocation, because this way it walks all the parent context up
to the TopMemoryContext.
TBH, I don't think this opt-in tracking business has been thought
On Fri, Aug 8, 2014 at 4:16 AM, Jeff Davis pg...@j-davis.com wrote:
I wasn't able to reproduce your results on my machine. At -s 300, with
maintenance_work_mem set high enough to do internal sort, it took about
40s and I heard some disk activity, so I didn't think it was a valid
result. I went
On Fri, 2014-08-08 at 01:16 -0700, Jeff Davis wrote:
Either way, it's better to be conservative. Attached is a version of the
patch with opt-in memory usage tracking. Child contexts inherit the
setting from their parent.
There was a problem with the previous patch: the parent was unlinked
On Wed, 2014-08-06 at 11:43 -0400, Robert Haas wrote:
Comparing the median times, that's about a 3% regression. For this
particular case, we might be able to recapture that by replacing the
bespoke memory-tracking logic in tuplesort.c with use of this new
facility. I'm not sure whether there
On Sat, Aug 2, 2014 at 4:40 PM, Jeff Davis pg...@j-davis.com wrote:
Attached is a patch that explicitly tracks allocated memory (the blocks,
not the chunks) for each memory context, as well as its children.
This is a prerequisite for memory-bounded HashAgg, which I intend to
submit for the
On 2.8.2014 22:40, Jeff Davis wrote:
Attached is a patch that explicitly tracks allocated memory (the blocks,
not the chunks) for each memory context, as well as its children.
This is a prerequisite for memory-bounded HashAgg, which I intend to
Anyway, I'm really looking forward to the
On Sat, Aug 2, 2014 at 1:40 PM, Jeff Davis pg...@j-davis.com wrote:
This is a prerequisite for memory-bounded HashAgg, which I intend to
submit for the next CF.
FWIW, I think that's a good project. A large number of these TPC-H
queries used HashAggs when I checked, on a moderate sized sample
Attached is a patch that explicitly tracks allocated memory (the blocks,
not the chunks) for each memory context, as well as its children.
This is a prerequisite for memory-bounded HashAgg, which I intend to
submit for the next CF. Hashjoin tracks the tuple sizes that it adds to
the hash table,
51 matches
Mail list logo
