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-30T07:14:23Z
Lists: pgsql-hackers 
On Tue, 30 Aug 2022 at 03:16, Robert Haas <robertmhaas@gmail.com> wrote:
> ../../../../src/include/utils/memutils_memorychunk.h:186:18: error:
> comparison of constant 7 with expression of type
> 'MemoryContextMethodID' (aka 'enum MemoryContextMethodID') is always
> true [-Werror,-Wtautological-constant-out-of-range-compare]
>         Assert(methodid <= MEMORY_CONTEXT_METHODID_MASK);
>         ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/c.h:827:9: note: expanded from macro 'Assert'
>                 if (!(condition)) \
>                       ^~~~~~~~~
>
> I'm not sure what to do about that, but every file that includes
> memutils_memorychunk.h produces those warnings (which become errors
> due to -Werror).

I'm not really sure either. I tried compiling with clang 12.0.1 with
-Wtautological-constant-out-of-range-compare and don't get this
warning.

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.

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.

David

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