Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2015-02-22 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2015-02-22 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2015-01-07 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-30 Thread Jeff Janes
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-29 Thread Jim Nasby
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-28 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-28 Thread Peter Geoghegan
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-28 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-23 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-23 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-21 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-21 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-07 Thread Michael Paquier
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-12-01 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-30 Thread Peter Geoghegan
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-21 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-20 Thread Andres Freund
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 ---

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-18 Thread Jim Nasby
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Simon Riggs
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Simon Riggs
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Andres Freund
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Andres Freund
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-17 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-11-15 Thread Simon Riggs
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-10-18 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-10-15 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-29 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-29 Thread Tom Lane
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-29 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-22 Thread Robert Haas
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-22 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-22 Thread Robert Haas
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-22 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-20 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-19 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-19 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-19 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-16 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-16 Thread Tom Lane
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,

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-16 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-14 Thread Robert Haas
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-10 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-08 Thread Jeff Davis
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-06 Thread Robert Haas
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-06 Thread Tomas Vondra
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

Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg

2014-08-04 Thread Peter Geoghegan
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