Date: Sun, 29 Sep 2019 00:12:49 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>,
	Soumyadeep Chakraborty <sochakraborty@pivotal.io>,
	Robert Haas <robertmhaas@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>, soumyadeep2007@gmail.com
Subject: Re: Memory Accounting
Message-ID: <20190928221249.mo2jrtlakingnc5e@development>
References: <027a129b8525601c6a680d27ce3a7172dab61aab.camel@j-davis.com>
 <CAAKRu_aAtcpqFz0fJKAjrvgFUUmYp2mPbeXOPRKWNtJK_Fxa-g@mail.gmail.com>
 <20190724215228.tlyj74ie4l2jn7ji@development>
 <CADwEdor2jZitB2P94on2LUAhdi0iRHv2WNseV3LJmJPenuuNpw@mail.gmail.com>
 <67f66d2fa39e35aef90093c74531a38dcd5c0f73.camel@j-davis.com>
 <CAAKRu_b2Qxr=GzE7thk8_=JDgYRnOssYBuqWdZ39R2+Heo=oQw@mail.gmail.com>
 <20190926192244.mk5ru6nkofo7am3h@development>
 <57780817877d4b40106b248b63261e21a7a56261.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <57780817877d4b40106b248b63261e21a7a56261.camel@j-davis.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

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.

[1] https://www.postgresql.org/message-id/CA%2BTgmobnu7XEn1gRdXnFo37P79bF%3DqLt46%3D37ajP3Cro9dBRaA%40mail.gmail.com


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services