Fix bug in gistRelocateBuildBuffersOnSplit().

When we create a temporary copy of the old node buffer, in stack, we mustn't
leak that into any of the long-lived data structures. Before this patch,
when we called gistPopItupFromNodeBuffer(), it got added to the array of
"loaded buffers". After gistRelocateBuildBuffersOnSplit() exits, the
pointer added to the loaded buffers array points to garbage. Often that goes
unnotied, because when we go through the array of loaded buffers to unload
them, buffers with a NULL pageBuffer are ignored, which can often happen by
accident even if the pointer points to garbage.

This patch fixes that by marking the temporary copy in stack explicitly as
temporary, and refrain from adding buffers marked as temporary to the array
of loaded buffers.

While we're at it, initialize nodeBuffer->pageBlocknum to InvalidBlockNumber
and improve comments a bit. This isn't strictly necessary, but makes
debugging easier.

diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 34a12bc79c09b5292e2d01ad86345d865ac12191..a40b83857c139f1a5f0a5c4705863cc31def44d8 100644 (file)
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -148,8 +148,10 @@ gistGetNodeBuffer(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
        int         level;
        MemoryContext oldcxt = MemoryContextSwitchTo(gfbb->context);
 
-       nodeBuffer->pageBuffer = NULL;
+       /* nodeBuffer->nodeBlocknum is the hash key and was filled in already */
        nodeBuffer->blocksCount = 0;
+       nodeBuffer->pageBlocknum = InvalidBlockNumber;
+       nodeBuffer->pageBuffer = NULL;
        nodeBuffer->queuedForEmptying = false;
 
        /*
@@ -244,11 +246,15 @@ gistAllocateNewPageBuffer(GISTBuildBuffers *gfbb)
 }
 
 /*
- * Add specified block number into loadedBuffers array.
+ * Add specified buffer into loadedBuffers array.
  */
 static void
 gistAddLoadedBuffer(GISTBuildBuffers *gfbb, GISTNodeBuffer *nodeBuffer)
 {
+   /* Never add a temporary buffer to the array */
+   if (nodeBuffer->isTemp)
+       return;
+
    /* Enlarge the array if needed */
    if (gfbb->loadedBuffersCount >= gfbb->loadedBuffersLen)
    {
@@ -591,7 +597,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
                i;
    GISTENTRY   entry[INDEX_MAX_KEYS];
    bool        isnull[INDEX_MAX_KEYS];
-   GISTNodeBuffer nodebuf;
+   GISTNodeBuffer oldBuf;
    ListCell   *lc;
 
    /* If the splitted page doesn't have buffers, we have nothing to do. */
@@ -619,16 +625,14 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
     * read the tuples straight from the heap instead of the root buffer.
     */
    Assert(blocknum != GIST_ROOT_BLKNO);
-   memcpy(&nodebuf, nodeBuffer, sizeof(GISTNodeBuffer));
+   memcpy(&oldBuf, nodeBuffer, sizeof(GISTNodeBuffer));
+   oldBuf.isTemp = true;
 
    /* Reset the old buffer, used for the new left page from now on */
    nodeBuffer->blocksCount = 0;
    nodeBuffer->pageBuffer = NULL;
    nodeBuffer->pageBlocknum = InvalidBlockNumber;
 
-   /* Reassign pointer to the saved copy. */
-   nodeBuffer = &nodebuf;
-
    /*
     * Allocate memory for information about relocation buffers.
     */
@@ -675,7 +679,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
     * Loop through all index tuples on the buffer on the splitted page,
     * moving them to buffers on the new pages.
     */
-   while (gistPopItupFromNodeBuffer(gfbb, nodeBuffer, &itup))
+   while (gistPopItupFromNodeBuffer(gfbb, &oldBuf, &itup))
    {
        float       sum_grow,
                    which_grow[INDEX_MAX_KEYS];

diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 0d6b625b298fbf712fd2d05a48b72e11d7dd025a..481ef5f4519e595fe35c7073f6496cd852668539 100644 (file)
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -326,6 +326,9 @@ typedef struct
    /* is this buffer queued for emptying? */
    bool        queuedForEmptying;
 
+   /* is this a temporary copy, not in the hash table? */
+   bool        isTemp;
+
    struct GISTBufferingInsertStack *path;
 } GISTNodeBuffer;