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
