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

I wrote:
> So I'm still inclined to leave 001 and 010 both unused, but the
> reason why is different than I thought before.

Which leaves me with the attached proposed wording.

            regards, tom lane

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285..b1a3c74830 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -32,6 +32,11 @@
 #include "utils/memutils_internal.h"


+static void BogusFree(void *pointer);
+static void *BogusRealloc(void *pointer, Size size);
+static MemoryContext BogusGetChunkContext(void *pointer);
+static Size BogusGetChunkSpace(void *pointer);
+
 /*****************************************************************************
  *      GLOBAL MEMORY                                                             *
  *****************************************************************************/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
     [MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
     [MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
     [MCTX_SLAB_ID].is_empty = SlabIsEmpty,
-    [MCTX_SLAB_ID].stats = SlabStats
+    [MCTX_SLAB_ID].stats = SlabStats,
 #ifdef MEMORY_CONTEXT_CHECKING
-    ,[MCTX_SLAB_ID].check = SlabCheck
+    [MCTX_SLAB_ID].check = SlabCheck,
 #endif
+
+    /*
+     * Unused (as yet) IDs should have dummy entries here.  This allows us to
+     * fail cleanly if a bogus pointer is passed to pfree or the like.  It
+     * seems sufficient to provide routines for the methods that might get
+     * invoked from inspection of a chunk (see MCXT_METHOD calls below).
+     */
+
+    [MCTX_UNUSED1_ID].free_p = BogusFree,
+    [MCTX_UNUSED1_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED1_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED1_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED2_ID].free_p = BogusFree,
+    [MCTX_UNUSED2_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED2_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED2_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED3_ID].free_p = BogusFree,
+    [MCTX_UNUSED3_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED4_ID].free_p = BogusFree,
+    [MCTX_UNUSED4_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED5_ID].free_p = BogusFree,
+    [MCTX_UNUSED5_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
 };

 /*
@@ -125,6 +162,77 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
 #define MCXT_METHOD(pointer, method) \
     mcxt_methods[GetMemoryChunkMethodID(pointer)].method

+/*
+ * GetMemoryChunkMethodID
+ *        Return the MemoryContextMethodID from the uint64 chunk header which
+ *        directly precedes 'pointer'.
+ */
+static inline MemoryContextMethodID
+GetMemoryChunkMethodID(const void *pointer)
+{
+    uint64        header;
+
+    /*
+     * Try to detect bogus pointers handed to us, poorly though we can.
+     * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+     * allocated chunk.
+     */
+    Assert(pointer == (const void *) MAXALIGN(pointer));
+
+    header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+    return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
+}
+
+/*
+ * GetMemoryChunkHeader
+ *        Return the uint64 chunk header which directly precedes 'pointer'.
+ *
+ * This is only used after GetMemoryChunkMethodID, so no need for error checks.
+ */
+static inline uint64
+GetMemoryChunkHeader(const void *pointer)
+{
+    return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+}
+
+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer).  As a possible
+ * aid in debugging, we report the header word along with the pointer
+ * address (if we got here, there must be an accessible header word).
+ */
+static void
+BogusFree(void *pointer)
+{
+    elog(ERROR, "pfree called with invalid pointer %p (header 0x%016llx)",
+         pointer, (long long) GetMemoryChunkHeader(pointer));
+}
+
+static void *
+BogusRealloc(void *pointer, Size size)
+{
+    elog(ERROR, "repalloc called with invalid pointer %p (header 0x%016llx)",
+         pointer, (long long) GetMemoryChunkHeader(pointer));
+    return NULL;                /* keep compiler quiet */
+}
+
+static MemoryContext
+BogusGetChunkContext(void *pointer)
+{
+    elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p (header 0x%016llx)",
+         pointer, (long long) GetMemoryChunkHeader(pointer));
+    return NULL;                /* keep compiler quiet */
+}
+
+static Size
+BogusGetChunkSpace(void *pointer)
+{
+    elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p (header 0x%016llx)",
+         pointer, (long long) GetMemoryChunkHeader(pointer));
+    return 0;                    /* keep compiler quiet */
+}
+

 /*****************************************************************************
  *      EXPORTED ROUTINES                                                         *
diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index 377cee7a84..bc2cbdd506 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -74,15 +74,26 @@ extern void SlabCheck(MemoryContext context);
  * MemoryContextMethodID
  *        A unique identifier for each MemoryContext implementation which
  *        indicates the index into the mcxt_methods[] array. See mcxt.c.
- *        The maximum value for this enum is constrained by
- *        MEMORY_CONTEXT_METHODID_MASK.  If an enum value higher than that is
- *        required then MEMORY_CONTEXT_METHODID_BITS will need to be increased.
+ *
+ * For robust error detection, ensure that MemoryContextMethodID has a value
+ * for each possible bit-pattern of MEMORY_CONTEXT_METHODID_MASK, and make
+ * dummy entries for unused IDs in the mcxt_methods[] array.  We also try
+ * to avoid using bit-patterns as valid IDs if they are likely to occur in
+ * garbage data, or if they could falsely match on chunks that are really from
+ * malloc not palloc.  (We can't tell that for most malloc implementations,
+ * but it happens that glibc stores flag bits in the same place where we put
+ * the MemoryContextMethodID, so the possible values are predictable for it.)
  */
 typedef enum MemoryContextMethodID
 {
+    MCTX_UNUSED1_ID,            /* 000 occurs in never-used memory */
+    MCTX_UNUSED2_ID,            /* glibc malloc'd chunks usually match 001 */
+    MCTX_UNUSED3_ID,            /* glibc malloc'd chunks > 128kB match 010 */
     MCTX_ASET_ID,
     MCTX_GENERATION_ID,
     MCTX_SLAB_ID,
+    MCTX_UNUSED4_ID,            /* available */
+    MCTX_UNUSED5_ID                /* 111 occurs in wipe_mem'd memory */
 } MemoryContextMethodID;

 /*
@@ -104,27 +115,4 @@ extern void MemoryContextCreate(MemoryContext node,
                                 MemoryContext parent,
                                 const char *name);

-/*
- * GetMemoryChunkMethodID
- *        Return the MemoryContextMethodID from the uint64 chunk header which
- *        directly precedes 'pointer'.
- */
-static inline MemoryContextMethodID
-GetMemoryChunkMethodID(void *pointer)
-{
-    uint64        header;
-
-    /*
-     * Try to detect bogus pointers handed to us, poorly though we can.
-     * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
-     * allocated chunk.
-     */
-    Assert(pointer != NULL);
-    Assert(pointer == (void *) MAXALIGN(pointer));
-
-    header = *((uint64 *) ((char *) pointer - sizeof(uint64)));
-
-    return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
-}
-
 #endif                            /* MEMUTILS_INTERNAL_H */