Date: Tue, 1 Oct 2019 04:11:14 +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: <20191001021114.iimd77xgnstaww7j@development>
References: 
 <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>
 <20190928221249.mo2jrtlakingnc5e@development>
 <20190928222206.thidzeygazl7axhg@development>
 <392e034c1b674a1a58c217dc440ea6197081ccfa.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <392e034c1b674a1a58c217dc440ea6197081ccfa.camel@j-davis.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Mon, Sep 30, 2019 at 01:34:13PM -0700, Jeff Davis wrote:
>On Sun, 2019-09-29 at 00:22 +0200, Tomas Vondra wrote:
>> 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.
>> >
>
>Fixed.
>
>I tested with some custom use of memory contexts. The reason
>AllocSetDelete() didn't fail before is that most memory contexts use
>the free lists (the list of free memory contexts, not the free list of
>chunks), so you need to specify a non-default minsize in order to
>prevent that and trigger the bug.
>
>AllocSetReset() worked, but it was reading the header of the keeper
>block after wiping the contents of the keeper block. It technically
>worked, because the header of the keeper block was not wiped, but it
>seems more clear to explicitly save the size of the keeper block. In
>AllocSetDelete(), saving the keeper size is required, because it wipes
>the block headers in addition to the contents.
>

OK, looks good to me now.

>> Oh, and one more thing - this probably needs to add at least some
>> basic
>> explanation of the accounting to src/backend/mmgr/README.
>
>Added.
>

Well, I've meant a couple of paragraphs explaining the motivation, and
relevant trade-offs considered. So I've written a brief summary of the
design as I understand it and pushed it like that. Of course, if you
could proof-read it, that'd be good.

I had a bit of a hard time deciding who to list as a reviewer - this
patch started sometime in ~2015, and it was initially discussed as part
of the larger hashagg effort, with plenty of people discussing various
ancient versions of the patch. In the end I've included just people from
the current thread. If that omits important past reviews, I'm sorry.

For the record, results of the benchmarks I've done over the past couple
of days are in [1]. It includes both the reindex benchmark used by by
Robert in 2015, and a regular read-only pgbench. In general, the
overhead of the accounting is pretty much indistinguishable from noise
(at least on those two machines).

In any case, thanks for the perseverance working on this.


[1] https://github.com/tvondra/memory-accounting-benchmarks


regards

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