From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | Jeff Davis <pgsql(at)j-davis(dot)com> |
Cc: | Melanie Plageman <melanieplageman(at)gmail(dot)com>, Soumyadeep Chakraborty <sochakraborty(at)pivotal(dot)io>, Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, soumyadeep2007(at)gmail(dot)com |
Subject: | Re: Memory Accounting |
Date: | 2019-09-28 22:12:49 |
Message-ID: | 20190928221249.mo2jrtlakingnc5e@development |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Sep 26, 2019 at 01:36:46PM -0700, Jeff Davis wrote:
>On Thu, 2019-09-26 at 21:22 +0200, Tomas Vondra wrote:
>> It's worth mentioning that those bechmarks (I'm assuming we're
>> talking
>> about the numbers Rober shared in [1]) were done on patches that used
>> the eager accounting approach (i.e. walking all parent contexts and
>> updating the accounting for them).
>>
>> I'm pretty sure the current "lazy accounting" patches don't have that
>> issue, so unless someone objects and/or can show numbers
>> demonstrating
>> I'wrong I'll stick to my plan to get this committed soon.
>
>That was my conclusion, as well.
>
I was about to commit the patch, but during the final review I've
noticed two places that I think are bugs:
1) aset.c (AllocSetDelete)
--------------------------
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
if (block != set->keeper)
{
context->mem_allocated -= block->endptr - ((char *) block);
free(block);
}
2) generation.c (GenerationReset)
---------------------------------
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->blksize);
#endif
context->mem_allocated -= block->blksize;
Notice that when CLOBBER_FREED_MEMORY is defined, the code first calls
wipe_mem and then accesses fields of the (wiped) block. Interesringly
enough, the regression tests don't seem to exercise these bits - I've
tried adding elog(ERROR) and it still passes. For (2) that's not very
surprising because Generation context is only really used in logical
decoding (and we don't delete the context I think). Not sure about (1)
but it might be because AllocSetReset does the right thing and only
leaves behind the keeper block.
I'm pretty sure a custom function calling the contexts explicitly would
fall over, but I haven't tried.
Aside from that, I've repeated the REINDEX benchmarks done by Robert in
[1] with different scales on two different machines, and I've measured
no difference. Both machines are x86_64, I don't have access to any
Power machine at the moment, unfortunately.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2019-09-28 22:22:06 | Re: Memory Accounting |
Previous Message | Andrew Gierth | 2019-09-28 21:30:59 | Re: Possible bug: SQL function parameter in window frame definition |