Re: Reducing the chunk header sizes on all memory context types - Mailing list pgsql-hackers

I wrote:
> What I am mainly wondering about at this point is whether Asserts
> are good enough or we should use actual test-and-elog checks for
> these things.

Hearing no comments on that, I decided that a good policy would be
to use Asserts in the paths dealing with small chunks but test-and-elog
in the paths dealing with large chunks.  We already had test-and-elog
sanity checks in the latter paths, at least in aset.c, which the new
checks can reasonably be combined with.  It's unlikely that the
large-chunk case is at all performance-critical, too.  But adding
runtime checks in the small-chunk paths would probably risk losing
some performance in production builds, and I'm not currently prepared
to argue that the problem is big enough to justify that.

Hence v2 attached, which cleans things up a tad in aset.c and then
extends similar policy to generation.c and slab.c.  Of note is
that slab.c was doing things like this:

    SlabContext *slab = castNode(SlabContext, context);

    Assert(slab);

which has about the same effect as what I'm proposing with
AllocSetIsValid, but (a) it's randomly different from the
other allocators, and (b) it's probably a hair less efficient,
because I doubt the compiler can optimize away castNode's
special handling of NULL.  So I made these bits follow the
style of aset.c.

            regards, tom lane

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..db402e3a41 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
 #define GetFreeListLink(chkptr) \
     (AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)

+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+    ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
 /* Determine the size of the chunk based on the freelist index */
 #define GetChunkSizeFromFreeListIdx(fidx) \
     ((((Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
  * AllocSetIsValid
  *        True iff set is valid allocation set.
  */
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+    (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ *        True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+    (PointerIsValid(block) && AllocSetIsValid((block)->aset))

 /*
  * We always store external chunks on a dedicated block.  This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
 {
     AllocSet    set = (AllocSet) context;
     AllocBlock    block;
-    Size        keepersize PG_USED_FOR_ASSERTS_ONLY
-    = set->keeper->endptr - ((char *) set);
+    Size        keepersize PG_USED_FOR_ASSERTS_ONLY;

     AssertArg(AllocSetIsValid(set));

@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
     AllocSetCheck(context);
 #endif

+    /* Remember keeper block size for Assert below */
+    keepersize = set->keeper->endptr - ((char *) set);
+
     /* Clear chunk freelists */
     MemSetAligned(set->freelist, 0, sizeof(set->freelist));

@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
 {
     AllocSet    set = (AllocSet) context;
     AllocBlock    block = set->blocks;
-    Size        keepersize PG_USED_FOR_ASSERTS_ONLY
-    = set->keeper->endptr - ((char *) set);
+    Size        keepersize PG_USED_FOR_ASSERTS_ONLY;

     AssertArg(AllocSetIsValid(set));

@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
     AllocSetCheck(context);
 #endif

+    /* Remember keeper block size for Assert below */
+    keepersize = set->keeper->endptr - ((char *) set);
+
     /*
      * If the context is a candidate for a freelist, put it into that freelist
      * instead of destroying it.
@@ -994,9 +1010,16 @@ AllocSetFree(void *pointer)

     if (MemoryChunkIsExternal(chunk))
     {
-
+        /* Release single-chunk block. */
         AllocBlock    block = ExternalChunkGetBlock(chunk);

+        /*
+         * Try to verify that we have a sane block pointer: the block header
+         * should reference an aset and the freeptr should match the endptr.
+         */
+        if (!AllocBlockIsValid(block) || block->freeptr != block->endptr)
+            elog(ERROR, "could not find block containing chunk %p", chunk);
+
         set = block->aset;

 #ifdef MEMORY_CONTEXT_CHECKING
@@ -1011,14 +1034,6 @@ AllocSetFree(void *pointer)
         }
 #endif

-
-        /*
-         * Try to verify that we have a sane block pointer, the freeptr should
-         * match the endptr.
-         */
-        if (block->freeptr != block->endptr)
-            elog(ERROR, "could not find block containing chunk %p", chunk);
-
         /* OK, remove block from aset's list and free it */
         if (block->prev)
             block->prev->next = block->next;
@@ -1036,12 +1051,23 @@ AllocSetFree(void *pointer)
     }
     else
     {
-        int            fidx = MemoryChunkGetValue(chunk);
         AllocBlock    block = MemoryChunkGetBlock(chunk);
-        AllocFreeListLink *link = GetFreeListLink(chunk);
+        int            fidx;
+        AllocFreeListLink *link;

+        /*
+         * In this path, for speed reasons we just Assert that the referenced
+         * block is good.  We can also Assert that the value field is sane.
+         * Future field experience may show that these Asserts had better
+         * become regular runtime test-and-elog checks.
+         */
+        AssertArg(AllocBlockIsValid(block));
         set = block->aset;

+        fidx = MemoryChunkGetValue(chunk);
+        Assert(FreeListIdxIsValid(fidx));
+        link = GetFreeListLink(chunk);
+
 #ifdef MEMORY_CONTEXT_CHECKING
         /* Test for someone scribbling on unused space in chunk */
         if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
@@ -1089,6 +1115,7 @@ AllocSetRealloc(void *pointer, Size size)
     AllocSet    set;
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     Size        oldsize;
+    int            fidx;

     /* Allow access to private part of chunk header. */
     VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
@@ -1105,9 +1132,18 @@ AllocSetRealloc(void *pointer, Size size)
         Size        oldblksize;

         block = ExternalChunkGetBlock(chunk);
-        oldsize = block->endptr - (char *) pointer;
+
+        /*
+         * Try to verify that we have a sane block pointer: the block header
+         * should reference an aset and the freeptr should match the endptr.
+         */
+        if (!AllocBlockIsValid(block) || block->freeptr != block->endptr)
+            elog(ERROR, "could not find block containing chunk %p", chunk);
+
         set = block->aset;

+        oldsize = block->endptr - (char *) pointer;
+
 #ifdef MEMORY_CONTEXT_CHECKING
         /* Test for someone scribbling on unused space in chunk */
         Assert(chunk->requested_size < oldsize);
@@ -1116,13 +1152,6 @@ AllocSetRealloc(void *pointer, Size size)
                  set->header.name, chunk);
 #endif

-        /*
-         * Try to verify that we have a sane block pointer, the freeptr should
-         * match the endptr.
-         */
-        if (block->freeptr != block->endptr)
-            elog(ERROR, "could not find block containing chunk %p", chunk);
-
 #ifdef MEMORY_CONTEXT_CHECKING
         /* ensure there's always space for the sentinel byte */
         chksize = MAXALIGN(size + 1);
@@ -1201,9 +1230,20 @@ AllocSetRealloc(void *pointer, Size size)
     }

     block = MemoryChunkGetBlock(chunk);
-    oldsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
+
+    /*
+     * In this path, for speed reasons we just Assert that the referenced
+     * block is good. We can also Assert that the value field is sane. Future
+     * field experience may show that these Asserts had better become regular
+     * runtime test-and-elog checks.
+     */
+    AssertArg(AllocBlockIsValid(block));
     set = block->aset;

+    fidx = MemoryChunkGetValue(chunk);
+    Assert(FreeListIdxIsValid(fidx));
+    oldsize = GetChunkSizeFromFreeListIdx(fidx);
+
 #ifdef MEMORY_CONTEXT_CHECKING
     /* Test for someone scribbling on unused space in chunk */
     if (chunk->requested_size < oldsize)
@@ -1328,6 +1368,7 @@ AllocSetGetChunkContext(void *pointer)
     else
         block = (AllocBlock) MemoryChunkGetBlock(chunk);

+    AssertArg(AllocBlockIsValid(block));
     set = block->aset;

     return &set->header;
@@ -1342,16 +1383,19 @@ Size
 AllocSetGetChunkSpace(void *pointer)
 {
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+    int            fidx;

     if (MemoryChunkIsExternal(chunk))
     {
         AllocBlock    block = ExternalChunkGetBlock(chunk);

+        AssertArg(AllocBlockIsValid(block));
         return block->endptr - (char *) chunk;
     }

-    return GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk)) +
-        ALLOC_CHUNKHDRSZ;
+    fidx = MemoryChunkGetValue(chunk);
+    Assert(FreeListIdxIsValid(fidx));
+    return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
 }

 /*
@@ -1361,6 +1405,8 @@ AllocSetGetChunkSpace(void *pointer)
 bool
 AllocSetIsEmpty(MemoryContext context)
 {
+    AssertArg(AllocSetIsValid(context));
+
     /*
      * For now, we say "empty" only if the context is new or just reset. We
      * could examine the freelists to determine if all space has been freed,
@@ -1394,6 +1440,8 @@ AllocSetStats(MemoryContext context,
     AllocBlock    block;
     int            fidx;

+    AssertArg(AllocSetIsValid(set));
+
     /* Include context header in totalspace */
     totalspace = MAXALIGN(sizeof(AllocSetContext));

@@ -1405,14 +1453,14 @@ AllocSetStats(MemoryContext context,
     }
     for (fidx = 0; fidx < ALLOCSET_NUM_FREELISTS; fidx++)
     {
+        Size        chksz = GetChunkSizeFromFreeListIdx(fidx);
         MemoryChunk *chunk = set->freelist[fidx];

         while (chunk != NULL)
         {
-            Size        chksz = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
             AllocFreeListLink *link = GetFreeListLink(chunk);

-            Assert(GetChunkSizeFromFreeListIdx(fidx) == chksz);
+            Assert(MemoryChunkGetValue(chunk) == fidx);

             freechunks++;
             freespace += chksz + ALLOC_CHUNKHDRSZ;
@@ -1522,7 +1570,13 @@ AllocSetCheck(MemoryContext context)
             }
             else
             {
-                chsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));    /* aligned chunk size */
+                int            fidx = MemoryChunkGetValue(chunk);
+
+                if (!FreeListIdxIsValid(fidx))
+                    elog(WARNING, "problem in alloc set %s: bad chunk size for chunk %p in block %p",
+                         name, chunk, block);
+
+                chsize = GetChunkSizeFromFreeListIdx(fidx); /* aligned chunk size */

                 /*
                  * Check the stored block offset correctly references this
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index c743b24fa7..4cb75f493f 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -105,11 +105,20 @@ struct GenerationBlock
  * simplicity.
  */
 #define GENERATIONCHUNK_PRIVATE_LEN    offsetof(MemoryChunk, hdrmask)
+
 /*
  * GenerationIsValid
- *        True iff set is valid allocation set.
+ *        True iff set is valid generation set.
+ */
+#define GenerationIsValid(set) \
+    (PointerIsValid(set) && IsA(set, GenerationContext))
+
+/*
+ * GenerationBlockIsValid
+ *        True iff block is valid block of generation set.
  */
-#define GenerationIsValid(set) PointerIsValid(set)
+#define GenerationBlockIsValid(block) \
+    (PointerIsValid(block) && GenerationIsValid((block)->context))

 /*
  * We always store external chunks on a dedicated block.  This makes fetching
@@ -345,6 +354,8 @@ GenerationAlloc(MemoryContext context, Size size)
     Size        chunk_size;
     Size        required_size;

+    AssertArg(GenerationIsValid(set));
+
 #ifdef MEMORY_CONTEXT_CHECKING
     /* ensure there's always space for the sentinel byte */
     chunk_size = MAXALIGN(size + 1);
@@ -625,6 +636,14 @@ GenerationFree(void *pointer)
     if (MemoryChunkIsExternal(chunk))
     {
         block = ExternalChunkGetBlock(chunk);
+
+        /*
+         * Try to verify that we have a sane block pointer: the block header
+         * should reference a generation context.
+         */
+        if (!GenerationBlockIsValid(block))
+            elog(ERROR, "could not find block containing chunk %p", chunk);
+
 #if defined(MEMORY_CONTEXT_CHECKING) || defined(CLOBBER_FREED_MEMORY)
         chunksize = block->endptr - (char *) pointer;
 #endif
@@ -632,6 +651,14 @@ GenerationFree(void *pointer)
     else
     {
         block = MemoryChunkGetBlock(chunk);
+
+        /*
+         * In this path, for speed reasons we just Assert that the referenced
+         * block is good.  Future field experience may show that this Assert
+         * had better become a regular runtime test-and-elog check.
+         */
+        AssertArg(GenerationBlockIsValid(block));
+
 #if defined(MEMORY_CONTEXT_CHECKING) || defined(CLOBBER_FREED_MEMORY)
         chunksize = MemoryChunkGetValue(chunk);
 #endif
@@ -723,11 +750,27 @@ GenerationRealloc(void *pointer, Size size)
     if (MemoryChunkIsExternal(chunk))
     {
         block = ExternalChunkGetBlock(chunk);
+
+        /*
+         * Try to verify that we have a sane block pointer: the block header
+         * should reference a generation context.
+         */
+        if (!GenerationBlockIsValid(block))
+            elog(ERROR, "could not find block containing chunk %p", chunk);
+
         oldsize = block->endptr - (char *) pointer;
     }
     else
     {
         block = MemoryChunkGetBlock(chunk);
+
+        /*
+         * In this path, for speed reasons we just Assert that the referenced
+         * block is good.  Future field experience may show that this Assert
+         * had better become a regular runtime test-and-elog check.
+         */
+        AssertArg(GenerationBlockIsValid(block));
+
         oldsize = MemoryChunkGetValue(chunk);
     }

@@ -845,6 +888,7 @@ GenerationGetChunkContext(void *pointer)
     else
         block = (GenerationBlock *) MemoryChunkGetBlock(chunk);

+    AssertArg(GenerationBlockIsValid(block));
     return &block->context->header;
 }

@@ -863,6 +907,7 @@ GenerationGetChunkSpace(void *pointer)
     {
         GenerationBlock *block = ExternalChunkGetBlock(chunk);

+        AssertArg(GenerationBlockIsValid(block));
         chunksize = block->endptr - (char *) pointer;
     }
     else
@@ -881,6 +926,8 @@ GenerationIsEmpty(MemoryContext context)
     GenerationContext *set = (GenerationContext *) context;
     dlist_iter    iter;

+    AssertArg(GenerationIsValid(set));
+
     dlist_foreach(iter, &set->blocks)
     {
         GenerationBlock *block = dlist_container(GenerationBlock, node, iter.cur);
@@ -917,6 +964,8 @@ GenerationStats(MemoryContext context,
     Size        freespace = 0;
     dlist_iter    iter;

+    AssertArg(GenerationIsValid(set));
+
     /* Include context header in totalspace */
     totalspace = MAXALIGN(sizeof(GenerationContext));

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 9149aaafcb..f4f40007a4 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -111,6 +111,21 @@ typedef struct SlabBlock
 #define SlabChunkIndex(slab, block, chunk)    \
     (((char *) chunk - SlabBlockStart(block)) / slab->fullChunkSize)

+/*
+ * SlabIsValid
+ *        True iff set is valid slab allocation set.
+ */
+#define SlabIsValid(set) \
+    (PointerIsValid(set) && IsA(set, SlabContext))
+
+/*
+ * SlabBlockIsValid
+ *        True iff block is valid block of slab allocation set.
+ */
+#define SlabBlockIsValid(block) \
+    (PointerIsValid(block) && SlabIsValid((block)->slab))
+
+
 /*
  * SlabContextCreate
  *        Create a new Slab context.
@@ -236,10 +251,10 @@ SlabContextCreate(MemoryContext parent,
 void
 SlabReset(MemoryContext context)
 {
+    SlabContext *slab = (SlabContext *) context;
     int            i;
-    SlabContext *slab = castNode(SlabContext, context);

-    Assert(slab);
+    AssertArg(SlabIsValid(slab));

 #ifdef MEMORY_CONTEXT_CHECKING
     /* Check for corruption and leaks before freeing */
@@ -293,12 +308,12 @@ SlabDelete(MemoryContext context)
 void *
 SlabAlloc(MemoryContext context, Size size)
 {
-    SlabContext *slab = castNode(SlabContext, context);
+    SlabContext *slab = (SlabContext *) context;
     SlabBlock  *block;
     MemoryChunk *chunk;
     int            idx;

-    Assert(slab);
+    AssertArg(SlabIsValid(slab));

     Assert((slab->minFreeChunks >= 0) &&
            (slab->minFreeChunks < slab->chunksPerBlock));
@@ -450,10 +465,18 @@ SlabAlloc(MemoryContext context, Size size)
 void
 SlabFree(void *pointer)
 {
-    int            idx;
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     SlabBlock  *block = MemoryChunkGetBlock(chunk);
-    SlabContext *slab = block->slab;
+    SlabContext *slab;
+    int            idx;
+
+    /*
+     * Try to verify that we have a sane block pointer: the block header
+     * should reference a slab context.
+     */
+    if (!SlabBlockIsValid(block))
+        elog(ERROR, "could not find block containing chunk %p", chunk);
+    slab = block->slab;

 #ifdef MEMORY_CONTEXT_CHECKING
     /* Test for someone scribbling on unused space in chunk */
@@ -540,9 +563,16 @@ SlabRealloc(void *pointer, Size size)
 {
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     SlabBlock  *block = MemoryChunkGetBlock(chunk);
-    SlabContext *slab = block->slab;
+    SlabContext *slab;
+
+    /*
+     * Try to verify that we have a sane block pointer: the block header
+     * should reference a slab context.
+     */
+    if (!SlabBlockIsValid(block))
+        elog(ERROR, "could not find block containing chunk %p", chunk);
+    slab = block->slab;

-    Assert(slab);
     /* can't do actual realloc with slab, but let's try to be gentle */
     if (size == slab->chunkSize)
         return pointer;
@@ -560,11 +590,9 @@ SlabGetChunkContext(void *pointer)
 {
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     SlabBlock  *block = MemoryChunkGetBlock(chunk);
-    SlabContext *slab = block->slab;
-
-    Assert(slab != NULL);

-    return &slab->header;
+    AssertArg(SlabBlockIsValid(block));
+    return &block->slab->header;
 }

 /*
@@ -577,9 +605,10 @@ SlabGetChunkSpace(void *pointer)
 {
     MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
     SlabBlock  *block = MemoryChunkGetBlock(chunk);
-    SlabContext *slab = block->slab;
+    SlabContext *slab;

-    Assert(slab);
+    AssertArg(SlabBlockIsValid(block));
+    slab = block->slab;

     return slab->fullChunkSize;
 }
@@ -591,9 +620,9 @@ SlabGetChunkSpace(void *pointer)
 bool
 SlabIsEmpty(MemoryContext context)
 {
-    SlabContext *slab = castNode(SlabContext, context);
+    SlabContext *slab = (SlabContext *) context;

-    Assert(slab);
+    AssertArg(SlabIsValid(slab));

     return (slab->nblocks == 0);
 }
@@ -613,13 +642,15 @@ SlabStats(MemoryContext context,
           MemoryContextCounters *totals,
           bool print_to_stderr)
 {
-    SlabContext *slab = castNode(SlabContext, context);
+    SlabContext *slab = (SlabContext *) context;
     Size        nblocks = 0;
     Size        freechunks = 0;
     Size        totalspace;
     Size        freespace = 0;
     int            i;

+    AssertArg(SlabIsValid(slab));
+
     /* Include context header in totalspace */
     totalspace = slab->headerSize;

@@ -672,11 +703,11 @@ SlabStats(MemoryContext context,
 void
 SlabCheck(MemoryContext context)
 {
+    SlabContext *slab = (SlabContext *) context;
     int            i;
-    SlabContext *slab = castNode(SlabContext, context);
     const char *name = slab->header.name;

-    Assert(slab);
+    AssertArg(SlabIsValid(slab));
     Assert(slab->chunksPerBlock > 0);

     /* walk all the freelists */