On Tue, 6 Sept 2022 at 12:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > On Tue, 6 Sept 2022 at 11:09, Andres Freund <andres@anarazel.de> wrote:
> >> I was looking at
> >> MemoryContextContains(). Unless I am missing something, the patch omitted
> >> adjusting that? We'll probably always return false right now.
>
> > Oops. Yes. I'll push a fix a bit later.
I think the fix is harder than I thought, or perhaps impossible to do
given how we now determine the owning MemoryContext of a pointer.
There's a comment in MemoryContextContains which says:
* NB: Can't use GetMemoryChunkContext() here - that performs assertions
* that aren't acceptable here since we might be passed memory not
* allocated by any memory context.
That seems to indicate that we should be able to handle any random
pointer given to us (!). That comment seems more confident that'll
work than the function's header comment does:
* Caution: this test is reliable as long as 'pointer' does point to
* a chunk of memory allocated from *some* context. If 'pointer' points
* at memory obtained in some other way, there is a small chance of a
* false-positive result, since the bits right before it might look like
* a valid chunk header by chance.
Here that's just claiming the test might not be reliable and could
return false-positive results.
I find this entire function pretty scary as even before the context
changes that function seems to think it's fine to subtract sizeof(void
*) from the given pointer and dereference that memory. That could very
well segfault.
I wonder if there are many usages of MemoryContextContains in
extensions. If there's not, I'd be much happier if we got rid of this
function and used GetMemoryChunkContext() in nodeAgg.c and
nodeWindowAgg.c.
> +1 for adding something to regress.c that verifies that this
> works properly for all three allocators. I suggest making
> three contexts and cross-checking the correct results for
> all combinations of chunk A vs context B.
I went as far as adding an Assert to palloc(). I'm not quite sure what
you have in mind in regress.c
Attached is a draft patch. I just don't like this function one bit.
David
