Horton

Re: Reducing the chunk header sizes on all memory context types

David Rowley <dgrowleyml@gmail.com>

View thread View on postgr.es
From: David Rowley <dgrowleyml@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Amit Kapila <amit.kapila16@gmail.com>, Andres Freund <andres@anarazel.de>, Yura Sokolov <y.sokolov@postgrespro.ru>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Date: 2022-08-30T15:25:52Z
Lists: pgsql-hackers 
On Wed, 31 Aug 2022 at 03:00, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Aug 30, 2022 at 3:14 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > I think the Assert is useful as if we were ever to add an enum member
> > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS
> > then bad things would happen inside MemoryChunkSetHdrMask() and
> > MemoryChunkSetHdrMaskExternal().  I think it's unlikely we'll ever get
> > that many MemoryContext types, but I don't know for sure and would
> > rather the person who adds the 9th one get alerted to the lack of bit
> > space in MemoryChunk as soon as possible.
>
> Well I don't have a problem with that, but I think we should try to do
> it without causing compiler warnings. The attached patch fixes it for
> me.

I'm fine with adding the int cast. Seems like a good idea.

> > As much as I'm not a fan of adding new warnings for compiler options
> > that are not part of our standard set, I feel like if there are
> > warning flags out there that are as giving us false warnings such as
> > this one, then we shouldn't trouble ourselves trying to get rid of
> > them, especially so when they force us to remove something which might
> > catch a future bug.
>
> For me the point is that, at least on the compiler that I'm using, the
> warning suggests that the compiler will optimize the test away
> completely, and therefore it wouldn't catch a future bug. Could there
> be compilers where no warning is generated but the assertion is still
> optimized away?

I'd not considered that the compiler might optimise it away.  My
suspicions had been more along the lines of that clang removed the
enum out of range warnings because they were annoying and wrong as
it's pretty easy to set an enum variable to something out of range of
the defined enum values.

Looking at [1], it seems like 5.0.2 is producing the correct code and
it's just producing a warning. The 2nd compiler window has -Werror and
shows that it does fail to compile. If I change that to use clang
6.0.0 then it works. It seems to fail all the way back to clang 3.1.
clang 3.0.0 works.

David

[1] https://godbolt.org/z/Gx388z5Ej

Commits

  1. Harden memory context allocators against bogus chunk pointers.

  2. Improve our ability to detect bogus pointers passed to pfree et al.

  3. Remove MemoryContextContains().

  4. Remove uses of MemoryContextContains in nodeAgg.c and nodeWindowAgg.c.

  5. Temporarily make MemoryContextContains return false

  6. Make MemoryContextContains work correctly again

  7. Make more effort to put a sentinel at the end of allocated memory

  8. Fix some possibly latent bugs in slab.c

  9. Various cleanups of the new memory context header code

  10. Revert "Add missing padding from MemoryChunk struct"

  11. Use MAXALIGN() in calculations using sizeof(SlabBlock)

  12. Add missing padding from MemoryChunk struct

  13. Improve performance of and reduce overheads of memory management