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
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 f
On Thu, Jan 8, 2015 at 4:07 AM, Tomas Vondra
wrote:
> which is IMHO obviously wrong, because that accounts only for the
> hashtable itself.
> In those cases the patch actually does no memory accounting and as
> hashcontext has no child contexts, there's no accounting overhead.
> [blah]
> So the p
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:
>>
On Sun, Dec 28, 2014 at 12:45 PM, Peter Geoghegan wrote:
> On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis 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 12/28/14, 2:45 PM, Peter Geoghegan wrote:
On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis 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 p
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
On Sun, Dec 28, 2014 at 12:37 PM, Jeff Davis 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 my numbers?
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 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 i
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 compil
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_a
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 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 val
On Mon, Dec 8, 2014 at 9:39 AM, Michael Paquier
wrote:
> On Tue, Dec 2, 2014 at 2:14 PM, 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 wrote:
>>> > I can also just move isReset there, and keep mem_allocated as a ui
On Tue, Dec 2, 2014 at 2:14 PM, 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 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 Sun, 2014-11-30 at 17:49 -0800, Peter Geoghegan wrote:
> On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis 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
On Mon, Nov 17, 2014 at 11:39 PM, Jeff Davis 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 any such optimi
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
>> AllocSetConte
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/mem
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 mig
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
MemoryContextDa
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
***
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 Memo
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;
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
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 15.11.2014 22:36, Simon Riggs wrote:
> On 16 October 2014 02:26, Jeff Davis 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
On 17 November 2014 07:31, Jeff Davis 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 good sampling r
On 17 November 2014 07: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 all
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 malloc).
It was a surprise to me that accou
On 16 October 2014 02:26, Jeff Davis 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 even more perfo
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 an
On 29.8.2014 16:12, Tom Lane wrote:
> Jeff Davis 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, w
Jeff Davis 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 allocations anywhere
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 ca
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 Fri, Aug 22, 2014 at 2:13 PM, Tomas Vondra 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 flag had the
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 t
On Wed, Aug 20, 2014 at 2:11 AM, Jeff Davis 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 machine the resu
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 ca
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 th
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 walki
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 16.8.2014 20:00, Tom Lane wrote:
> Tomas Vondra 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
> t
Tomas Vondra 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, what happ
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
On Fri, Aug 8, 2014 at 4:16 AM, Jeff Davis 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 down to -s 15
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
be
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 th
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 me
On Sat, Aug 2, 2014 at 4:40 PM, 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
> submit for the next CF. Hashjo
On Sat, Aug 2, 2014 at 1:40 PM, Jeff Davis 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 TPC-H
database:
h
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, wh
54 matches
Mail list logo
