Fix possible duplicate tuples while GiST scan. Now page is processed

at once and ItemPointers are collected in memory.

Remove tuple's killing by killtuple() if tuple was moved to another
page - it could produce unaceptable overhead.

Backpatch up to 8.1 because the bug was introduced by GiST's concurrency support.

diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index a1fd296dbf25f01b547389adfd73423941e80755..97ae1cfd5857115bf87bc1363091b67f6d065eb0 100644 (file)
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/access/gist/gistget.c,v 1.74 2008/06/19 00:46:03 alvherre Exp $
+ *   $PostgreSQL: pgsql/src/backend/access/gist/gistget.c,v 1.75 2008/08/23 10:37:24 teodor Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -32,63 +32,39 @@ static bool gistindex_keytest(IndexTuple tuple, IndexScanDesc scan,
 static void
 killtuple(Relation r, GISTScanOpaque so, ItemPointer iptr)
 {
-   Buffer      buffer = so->curbuf;
-
-   for (;;)
-   {
-       Page        p;
-       BlockNumber blkno;
-       OffsetNumber offset,
-                   maxoff;
-
-       LockBuffer(buffer, GIST_SHARE);
-       gistcheckpage(r, buffer);
-       p = (Page) BufferGetPage(buffer);
+   Page        p;
+   OffsetNumber offset;
 
-       if (buffer == so->curbuf && XLByteEQ(so->stack->lsn, PageGetLSN(p)))
-       {
-           /* page unchanged, so all is simple */
-           offset = ItemPointerGetOffsetNumber(iptr);
-           ItemIdMarkDead(PageGetItemId(p, offset));
-           SetBufferCommitInfoNeedsSave(buffer);
-           LockBuffer(buffer, GIST_UNLOCK);
-           break;
-       }
+   LockBuffer(so->curbuf, GIST_SHARE);
+   gistcheckpage(r, so->curbuf);
+   p = (Page) BufferGetPage(so->curbuf);
 
-       maxoff = PageGetMaxOffsetNumber(p);
+   if (XLByteEQ(so->stack->lsn, PageGetLSN(p)))
+   {
+       /* page unchanged, so all is simple */
+       offset = ItemPointerGetOffsetNumber(iptr);
+       ItemIdMarkDead(PageGetItemId(p, offset));
+       SetBufferCommitInfoNeedsSave(so->curbuf);
+   }
+   else
+   {
+       OffsetNumber maxoff = PageGetMaxOffsetNumber(p);
 
        for (offset = FirstOffsetNumber; offset <= maxoff; offset = OffsetNumberNext(offset))
        {
-           IndexTuple  ituple = (IndexTuple) PageGetItem(p, PageGetItemId(p, offset));
+               IndexTuple  ituple = (IndexTuple) PageGetItem(p, PageGetItemId(p, offset));
 
            if (ItemPointerEquals(&(ituple->t_tid), iptr))
            {
                /* found */
                ItemIdMarkDead(PageGetItemId(p, offset));
-               SetBufferCommitInfoNeedsSave(buffer);
-               LockBuffer(buffer, GIST_UNLOCK);
-               if (buffer != so->curbuf)
-                   ReleaseBuffer(buffer);
-               return;
+               SetBufferCommitInfoNeedsSave(so->curbuf);
+               break;
            }
        }
-
-       /* follow right link */
-
-       /*
-        * ??? is it good? if tuple dropped by concurrent vacuum, we will read
-        * all leaf pages...
-        */
-       blkno = GistPageGetOpaque(p)->rightlink;
-       LockBuffer(buffer, GIST_UNLOCK);
-       if (buffer != so->curbuf)
-           ReleaseBuffer(buffer);
-
-       if (blkno == InvalidBlockNumber)
-           /* can't found, dropped by somebody else */
-           return;
-       buffer = ReadBuffer(r, blkno);
    }
+
+   LockBuffer(so->curbuf, GIST_UNLOCK);
 }
 
 /*
@@ -154,7 +130,6 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
    GISTSearchStack *stk;
    IndexTuple  it;
    GISTPageOpaque opaque;
-   bool        resetoffset = false;
    int64       ntids = 0;
 
    so = (GISTScanOpaque) scan->opaque;
@@ -179,6 +154,50 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
        return 0;
    }
 
+   /*
+    * check stored pointers from last visit 
+    */
+   if ( so->nPageData > 0 ) 
+   {
+       /*
+        * gistgetmulti never should go here
+        */
+       Assert( tbm == NULL );
+
+       if ( so->curPageData < so->nPageData )
+       {
+           /*
+            * pageData is already ordered for scan's direction
+            */
+           scan->xs_ctup.t_self = so->pageData[ so->curPageData ].iptr;
+           scan->xs_recheck = so->pageData[ so->curPageData ].recheck;
+           so->curPageData ++;
+
+           return 1;
+       }
+       else
+       {
+           /*
+            * Go to the next page
+            */
+           stk = so->stack->next;
+           pfree(so->stack);
+           so->stack = stk;
+
+           /* If we're out of stack entries, we're done */
+           if (so->stack == NULL)
+           {
+               ReleaseBuffer(so->curbuf);
+               so->curbuf = InvalidBuffer;
+               return 0;
+           }
+
+           so->curbuf = ReleaseAndReadBuffer(so->curbuf,
+                                             scan->indexRelation,
+                                             stk->block);
+       }
+   }
+
    for (;;)
    {
        CHECK_FOR_INTERRUPTS();
@@ -189,30 +208,25 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
        gistcheckpage(scan->indexRelation, so->curbuf);
        p = BufferGetPage(so->curbuf);
        opaque = GistPageGetOpaque(p);
-       resetoffset = false;
 
-       if (XLogRecPtrIsInvalid(so->stack->lsn) || !XLByteEQ(so->stack->lsn, PageGetLSN(p)))
-       {
-           /* first visit or page changed from last visit, reset offset */
-           so->stack->lsn = PageGetLSN(p);
-           resetoffset = true;
-
-           /* check page split, occured from last visit or visit to parent */
-           if (!XLogRecPtrIsInvalid(so->stack->parentlsn) &&
-               XLByteLT(so->stack->parentlsn, opaque->nsn) &&
-               opaque->rightlink != InvalidBlockNumber /* sanity check */ &&
-               (so->stack->next == NULL || so->stack->next->block != opaque->rightlink)        /* check if already
-                   added */ )
-           {
-               /* detect page split, follow right link to add pages */
+       /* remember lsn to identify page changed for tuple's killing */
+       so->stack->lsn = PageGetLSN(p);
 
-               stk = (GISTSearchStack *) palloc(sizeof(GISTSearchStack));
-               stk->next = so->stack->next;
-               stk->block = opaque->rightlink;
-               stk->parentlsn = so->stack->parentlsn;
-               memset(&(stk->lsn), 0, sizeof(GistNSN));
-               so->stack->next = stk;
-           }
+       /* check page split, occured since visit to parent */
+       if (!XLogRecPtrIsInvalid(so->stack->parentlsn) &&
+           XLByteLT(so->stack->parentlsn, opaque->nsn) &&
+           opaque->rightlink != InvalidBlockNumber /* sanity check */ &&
+           (so->stack->next == NULL || so->stack->next->block != opaque->rightlink)        /* check if already
+               added */ )
+       {
+           /* detect page split, follow right link to add pages */
+
+           stk = (GISTSearchStack *) palloc(sizeof(GISTSearchStack));
+           stk->next = so->stack->next;
+           stk->block = opaque->rightlink;
+           stk->parentlsn = so->stack->parentlsn;
+           memset(&(stk->lsn), 0, sizeof(GistNSN));
+           so->stack->next = stk;
        }
 
        /* if page is empty, then just skip it */
@@ -235,25 +249,13 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
            continue;
        }
 
-       if (!GistPageIsLeaf(p) || resetoffset ||
-           !ItemPointerIsValid(&so->curpos))
-       {
-           if (ScanDirectionIsBackward(dir))
-               n = PageGetMaxOffsetNumber(p);
-           else
-               n = FirstOffsetNumber;
-       }
+       if (ScanDirectionIsBackward(dir))
+           n = PageGetMaxOffsetNumber(p);
        else
-       {
-           n = ItemPointerGetOffsetNumber(&(so->curpos));
-
-           if (ScanDirectionIsBackward(dir))
-               n = OffsetNumberPrev(n);
-           else
-               n = OffsetNumberNext(n);
-       }
+           n = FirstOffsetNumber;
 
        /* wonderful, we can look at page */
+       so->nPageData = so->curPageData = 0;
 
        for (;;)
        {
@@ -261,6 +263,18 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
 
            if (!OffsetNumberIsValid(n))
            {
+               /*
+                * If we was called from gistgettuple and current buffer contains
+                * something matched then make a recursive call - it will return
+                * ItemPointer from so->pageData. But we save buffer pinned to 
+                * support tuple's killing
+                */
+               if ( !tbm && so->nPageData > 0 )
+               {
+                   LockBuffer(so->curbuf, GIST_UNLOCK);
+                   return gistnext(scan, dir, NULL);
+               }
+
                /*
                 * We ran out of matching index entries on the current page,
                 * so pop the top stack entry and use it to continue the
@@ -306,11 +320,9 @@ gistnext(IndexScanDesc scan, ScanDirection dir, TIDBitmap *tbm)
                        tbm_add_tuples(tbm, &it->t_tid, 1, scan->xs_recheck);
                    else 
                    {
-                       scan->xs_ctup.t_self = it->t_tid;
-                       /* scan->xs_recheck is already set */
-
-                       LockBuffer(so->curbuf, GIST_UNLOCK);
-                       return ntids; /* always 1 */
+                       so->pageData[ so->nPageData ].iptr = it->t_tid;
+                       so->pageData[ so->nPageData ].recheck = scan->xs_recheck;
+                       so->nPageData ++;
                    }
                }
            }

diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 65833eabf71f5601b0a905cfc4399438cd4496fb..ac3ea446ded5543a33e62a9fd9df6e797a9afdcc 100644 (file)
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/access/gist/gistscan.c,v 1.70 2008/06/19 00:46:03 alvherre Exp $
+ *   $PostgreSQL: pgsql/src/backend/access/gist/gistscan.c,v 1.71 2008/08/23 10:37:24 teodor Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -63,6 +63,7 @@ gistrescan(PG_FUNCTION_ARGS)
            ReleaseBuffer(so->markbuf);
            so->markbuf = InvalidBuffer;
        }
+
    }
    else
    {
@@ -83,6 +84,7 @@ gistrescan(PG_FUNCTION_ARGS)
     */
    ItemPointerSetInvalid(&so->curpos);
    ItemPointerSetInvalid(&so->markpos);
+   so->nPageData = so->curPageData = 0;
 
    /* Update scan key, if a new one is given */
    if (key && scan->numberOfKeys > 0)
@@ -150,6 +152,11 @@ gistmarkpos(PG_FUNCTION_ARGS)
        so->markbuf = so->curbuf;
    }
 
+   so->markNPageData = so->nPageData;
+   so->markCurPageData = so->curPageData;
+   if ( so->markNPageData > 0 )
+       memcpy( so->markPageData, so->pageData, sizeof(ItemResult) * so->markNPageData );       
+
    PG_RETURN_VOID();
 }
 
@@ -199,6 +206,11 @@ gistrestrpos(PG_FUNCTION_ARGS)
        so->curbuf = so->markbuf;
    }
 
+   so->nPageData = so->markNPageData;
+   so->curPageData = so->markNPageData;
+   if ( so->markNPageData > 0 )
+       memcpy( so->pageData, so->markPageData, sizeof(ItemResult) * so->markNPageData );       
+
    PG_RETURN_VOID();
 }
 

diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 497ceb318ede1d3cebaf81483e8e4ea7579692c3..e366dcc53e7d91d516bf911282c76df85d141b73 100644 (file)
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/gist_private.h,v 1.31 2008/06/19 00:46:05 alvherre Exp $
+ * $PostgreSQL: pgsql/src/include/access/gist_private.h,v 1.32 2008/08/23 10:37:24 teodor Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -58,6 +58,12 @@ typedef struct GISTSTATE
    TupleDesc   tupdesc;
 } GISTSTATE;
 
+typedef struct ItemResult
+{
+   ItemPointerData     iptr;
+   bool                recheck;
+} ItemResult;
+
 /*
  * When we're doing a scan, we need to keep track of the parent stack
  * for the marked and current items.
@@ -73,6 +79,13 @@ typedef struct GISTScanOpaqueData
    ItemPointerData curpos;
    Buffer      markbuf;
    ItemPointerData markpos;
+
+   ItemResult      pageData[BLCKSZ/sizeof(IndexTupleData)];
+   OffsetNumber    nPageData;
+   OffsetNumber    curPageData;
+   ItemResult      markPageData[BLCKSZ/sizeof(IndexTupleData)];
+   OffsetNumber    markNPageData;
+   OffsetNumber    markCurPageData;
 } GISTScanOpaqueData;
 
 typedef GISTScanOpaqueData *GISTScanOpaque;