Re: [PATCHES] currentItemData refactoring - Mailing list pgsql-patches

Patch applied by Neil.  Thanks.

---------------------------------------------------------------------------


Heikki Linnakangas wrote:
> Here's a patch to remove currentItemData & currentMarkpos from
> IndexScanDesc, and add equivalent fields in access method specific
> opaque structs for those access methods that need them. That makes the
> index am API cleaner.
>
> --
>   Heikki Linnakangas
>   EnterpriseDB   http://www.enterprisedb.com
>

[ text/x-patch is unsupported, treating like TEXT/PLAIN ]

> Index: src/backend/access/gist/gistget.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/gist/gistget.c,v
> retrieving revision 1.60
> diff -c -r1.60 gistget.c
> *** src/backend/access/gist/gistget.c    14 Jul 2006 14:52:16 -0000    1.60
> --- src/backend/access/gist/gistget.c    12 Sep 2006 15:37:48 -0000
> ***************
> *** 106,113 ****
>        * If we have produced an index tuple in the past and the executor has
>        * informed us we need to mark it as "killed", do so now.
>        */
> !     if (scan->kill_prior_tuple && ItemPointerIsValid(&(scan->currentItemData)))
> !         killtuple(scan->indexRelation, so, &(scan->currentItemData));
>
>       /*
>        * Get the next tuple that matches the search key. If asked to skip killed
> --- 106,113 ----
>        * If we have produced an index tuple in the past and the executor has
>        * informed us we need to mark it as "killed", do so now.
>        */
> !     if (scan->kill_prior_tuple && ItemPointerIsValid(&(so->curpos)))
> !         killtuple(scan->indexRelation, so, &(so->curpos));
>
>       /*
>        * Get the next tuple that matches the search key. If asked to skip killed
> ***************
> *** 151,157 ****
>
>       so = (GISTScanOpaque) scan->opaque;
>
> !     if (ItemPointerIsValid(&scan->currentItemData) == false)
>       {
>           /* Being asked to fetch the first entry, so start at the root */
>           Assert(so->curbuf == InvalidBuffer);
> --- 151,157 ----
>
>       so = (GISTScanOpaque) scan->opaque;
>
> !     if (ItemPointerIsValid(&so->curpos) == false)
>       {
>           /* Being asked to fetch the first entry, so start at the root */
>           Assert(so->curbuf == InvalidBuffer);
> ***************
> *** 226,232 ****
>           }
>
>           if (!GistPageIsLeaf(p) || resetoffset ||
> !             !ItemPointerIsValid(&scan->currentItemData))
>           {
>               if (ScanDirectionIsBackward(dir))
>                   n = PageGetMaxOffsetNumber(p);
> --- 226,232 ----
>           }
>
>           if (!GistPageIsLeaf(p) || resetoffset ||
> !             !ItemPointerIsValid(&so->curpos))
>           {
>               if (ScanDirectionIsBackward(dir))
>                   n = PageGetMaxOffsetNumber(p);
> ***************
> *** 235,241 ****
>           }
>           else
>           {
> !             n = ItemPointerGetOffsetNumber(&(scan->currentItemData));
>
>               if (ScanDirectionIsBackward(dir))
>                   n = OffsetNumberPrev(n);
> --- 235,241 ----
>           }
>           else
>           {
> !             n = ItemPointerGetOffsetNumber(&(so->curpos));
>
>               if (ScanDirectionIsBackward(dir))
>                   n = OffsetNumberPrev(n);
> ***************
> *** 285,291 ****
>                    * we can efficiently resume the index scan later.
>                    */
>
> !                 ItemPointerSet(&(scan->currentItemData),
>                                  BufferGetBlockNumber(so->curbuf), n);
>
>                   if (!(ignore_killed_tuples && ItemIdDeleted(PageGetItemId(p, n))))
> --- 285,291 ----
>                    * we can efficiently resume the index scan later.
>                    */
>
> !                 ItemPointerSet(&(so->curpos),
>                                  BufferGetBlockNumber(so->curbuf), n);
>
>                   if (!(ignore_killed_tuples && ItemIdDeleted(PageGetItemId(p, n))))
> Index: src/backend/access/gist/gistscan.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/gist/gistscan.c,v
> retrieving revision 1.64
> diff -c -r1.64 gistscan.c
> *** src/backend/access/gist/gistscan.c    14 Jul 2006 14:52:16 -0000    1.64
> --- src/backend/access/gist/gistscan.c    12 Sep 2006 15:38:52 -0000
> ***************
> *** 42,53 ****
>       GISTScanOpaque so;
>       int            i;
>
> -     /*
> -      * Clear all the pointers.
> -      */
> -     ItemPointerSetInvalid(&scan->currentItemData);
> -     ItemPointerSetInvalid(&scan->currentMarkData);
> -
>       so = (GISTScanOpaque) scan->opaque;
>       if (so != NULL)
>       {
> --- 42,47 ----
> ***************
> *** 82,87 ****
> --- 76,88 ----
>           scan->opaque = so;
>       }
>
> +
> +     /*
> +      * Clear all the pointers.
> +      */
> +     ItemPointerSetInvalid(&so->curpos);
> +     ItemPointerSetInvalid(&so->markpos);
> +
>       /* Update scan key, if a new one is given */
>       if (key && scan->numberOfKeys > 0)
>       {
> ***************
> *** 111,117 ****
>                  *n,
>                  *tmp;
>
> !     scan->currentMarkData = scan->currentItemData;
>       so = (GISTScanOpaque) scan->opaque;
>       if (so->flags & GS_CURBEFORE)
>           so->flags |= GS_MRKBEFORE;
> --- 112,118 ----
>                  *n,
>                  *tmp;
>
> !     so->markpos = so->curpos;
>       so = (GISTScanOpaque) scan->opaque;
>       if (so->flags & GS_CURBEFORE)
>           so->flags |= GS_MRKBEFORE;
> ***************
> *** 160,166 ****
>                  *n,
>                  *tmp;
>
> !     scan->currentItemData = scan->currentMarkData;
>       so = (GISTScanOpaque) scan->opaque;
>       if (so->flags & GS_MRKBEFORE)
>           so->flags |= GS_CURBEFORE;
> --- 161,167 ----
>                  *n,
>                  *tmp;
>
> !     so->curpos = so->markpos;
>       so = (GISTScanOpaque) scan->opaque;
>       if (so->flags & GS_MRKBEFORE)
>           so->flags |= GS_CURBEFORE;
> Index: src/backend/access/hash/hash.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/hash/hash.c,v
> retrieving revision 1.91
> diff -c -r1.91 hash.c
> *** src/backend/access/hash/hash.c    14 Jul 2006 14:52:17 -0000    1.91
> --- src/backend/access/hash/hash.c    12 Sep 2006 15:45:32 -0000
> ***************
> *** 185,191 ****
>        * appropriate direction.  If we haven't done so yet, we call a routine to
>        * get the first item in the scan.
>        */
> !     if (ItemPointerIsValid(&(scan->currentItemData)))
>       {
>           /*
>            * Check to see if we should kill the previously-fetched tuple.
> --- 185,191 ----
>        * appropriate direction.  If we haven't done so yet, we call a routine to
>        * get the first item in the scan.
>        */
> !     if (ItemPointerIsValid(&(so->hashso_curpos)))
>       {
>           /*
>            * Check to see if we should kill the previously-fetched tuple.
> ***************
> *** 195,201 ****
>               /*
>                * Yes, so mark it by setting the LP_DELETE bit in the item flags.
>                */
> !             offnum = ItemPointerGetOffsetNumber(&(scan->currentItemData));
>               page = BufferGetPage(so->hashso_curbuf);
>               PageGetItemId(page, offnum)->lp_flags |= LP_DELETE;
>
> --- 195,201 ----
>               /*
>                * Yes, so mark it by setting the LP_DELETE bit in the item flags.
>                */
> !             offnum = ItemPointerGetOffsetNumber(&(so->hashso_curpos));
>               page = BufferGetPage(so->hashso_curbuf);
>               PageGetItemId(page, offnum)->lp_flags |= LP_DELETE;
>
> ***************
> *** 222,228 ****
>       {
>           while (res)
>           {
> !             offnum = ItemPointerGetOffsetNumber(&(scan->currentItemData));
>               page = BufferGetPage(so->hashso_curbuf);
>               if (!ItemIdDeleted(PageGetItemId(page, offnum)))
>                   break;
> --- 222,228 ----
>       {
>           while (res)
>           {
> !             offnum = ItemPointerGetOffsetNumber(&(so->hashso_curpos));
>               page = BufferGetPage(so->hashso_curbuf);
>               if (!ItemIdDeleted(PageGetItemId(page, offnum)))
>                   break;
> ***************
> *** 269,275 ****
>           /*
>            * Start scan, or advance to next tuple.
>            */
> !         if (ItemPointerIsValid(&(scan->currentItemData)))
>               res = _hash_next(scan, ForwardScanDirection);
>           else
>               res = _hash_first(scan, ForwardScanDirection);
> --- 269,275 ----
>           /*
>            * Start scan, or advance to next tuple.
>            */
> !         if (ItemPointerIsValid(&(so->hashso_curpos)))
>               res = _hash_next(scan, ForwardScanDirection);
>           else
>               res = _hash_first(scan, ForwardScanDirection);
> ***************
> *** 284,290 ****
>                   Page        page;
>                   OffsetNumber offnum;
>
> !                 offnum = ItemPointerGetOffsetNumber(&(scan->currentItemData));
>                   page = BufferGetPage(so->hashso_curbuf);
>                   if (!ItemIdDeleted(PageGetItemId(page, offnum)))
>                       break;
> --- 284,290 ----
>                   Page        page;
>                   OffsetNumber offnum;
>
> !                 offnum = ItemPointerGetOffsetNumber(&(so->hashso_curpos));
>                   page = BufferGetPage(so->hashso_curbuf);
>                   if (!ItemIdDeleted(PageGetItemId(page, offnum)))
>                       break;
> ***************
> *** 326,331 ****
> --- 326,335 ----
>       so->hashso_bucket_blkno = 0;
>       so->hashso_curbuf = so->hashso_mrkbuf = InvalidBuffer;
>       scan->opaque = so;
> +     /* set positions invalid (this will cause _hash_first call) */
> +     ItemPointerSetInvalid(&(so->hashso_curpos));
> +     ItemPointerSetInvalid(&(so->hashso_mrkpos));
> +
>
>       /* register scan in case we change pages it's using */
>       _hash_regscan(scan);
> ***************
> *** 360,370 ****
>           if (so->hashso_bucket_blkno)
>               _hash_droplock(rel, so->hashso_bucket_blkno, HASH_SHARE);
>           so->hashso_bucket_blkno = 0;
> -     }
>
> !     /* set positions invalid (this will cause _hash_first call) */
> !     ItemPointerSetInvalid(&(scan->currentItemData));
> !     ItemPointerSetInvalid(&(scan->currentMarkData));
>
>       /* Update scan key, if a new one is given */
>       if (scankey && scan->numberOfKeys > 0)
> --- 364,374 ----
>           if (so->hashso_bucket_blkno)
>               _hash_droplock(rel, so->hashso_bucket_blkno, HASH_SHARE);
>           so->hashso_bucket_blkno = 0;
>
> !         /* set positions invalid (this will cause _hash_first call) */
> !         ItemPointerSetInvalid(&(so->hashso_curpos));
> !         ItemPointerSetInvalid(&(so->hashso_mrkpos));
> !     }
>
>       /* Update scan key, if a new one is given */
>       if (scankey && scan->numberOfKeys > 0)
> ***************
> *** 406,415 ****
>           _hash_droplock(rel, so->hashso_bucket_blkno, HASH_SHARE);
>       so->hashso_bucket_blkno = 0;
>
> -     /* be tidy */
> -     ItemPointerSetInvalid(&(scan->currentItemData));
> -     ItemPointerSetInvalid(&(scan->currentMarkData));
> -
>       pfree(so);
>       scan->opaque = NULL;
>
> --- 410,415 ----
> ***************
> *** 430,443 ****
>       if (BufferIsValid(so->hashso_mrkbuf))
>           _hash_dropbuf(rel, so->hashso_mrkbuf);
>       so->hashso_mrkbuf = InvalidBuffer;
> !     ItemPointerSetInvalid(&(scan->currentMarkData));
>
> !     /* bump pin count on currentItemData and copy to currentMarkData */
> !     if (ItemPointerIsValid(&(scan->currentItemData)))
>       {
>           IncrBufferRefCount(so->hashso_curbuf);
>           so->hashso_mrkbuf = so->hashso_curbuf;
> !         scan->currentMarkData = scan->currentItemData;
>       }
>
>       PG_RETURN_VOID();
> --- 430,443 ----
>       if (BufferIsValid(so->hashso_mrkbuf))
>           _hash_dropbuf(rel, so->hashso_mrkbuf);
>       so->hashso_mrkbuf = InvalidBuffer;
> !     ItemPointerSetInvalid(&(so->hashso_mrkpos));
>
> !     /* bump pin count on current buffer and copy to marked buffer */
> !     if (ItemPointerIsValid(&(so->hashso_curpos)))
>       {
>           IncrBufferRefCount(so->hashso_curbuf);
>           so->hashso_mrkbuf = so->hashso_curbuf;
> !         so->hashso_mrkpos = so->hashso_curpos;
>       }
>
>       PG_RETURN_VOID();
> ***************
> *** 457,470 ****
>       if (BufferIsValid(so->hashso_curbuf))
>           _hash_dropbuf(rel, so->hashso_curbuf);
>       so->hashso_curbuf = InvalidBuffer;
> !     ItemPointerSetInvalid(&(scan->currentItemData));
>
> !     /* bump pin count on currentMarkData and copy to currentItemData */
> !     if (ItemPointerIsValid(&(scan->currentMarkData)))
>       {
>           IncrBufferRefCount(so->hashso_mrkbuf);
>           so->hashso_curbuf = so->hashso_mrkbuf;
> !         scan->currentItemData = scan->currentMarkData;
>       }
>
>       PG_RETURN_VOID();
> --- 457,470 ----
>       if (BufferIsValid(so->hashso_curbuf))
>           _hash_dropbuf(rel, so->hashso_curbuf);
>       so->hashso_curbuf = InvalidBuffer;
> !     ItemPointerSetInvalid(&(so->hashso_curpos));
>
> !     /* bump pin count on marked buffer and copy to current buffer */
> !     if (ItemPointerIsValid(&(so->hashso_mrkpos)))
>       {
>           IncrBufferRefCount(so->hashso_mrkbuf);
>           so->hashso_curbuf = so->hashso_mrkbuf;
> !         so->hashso_curpos = so->hashso_mrkpos;
>       }
>
>       PG_RETURN_VOID();
> Index: src/backend/access/hash/hashsearch.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/hash/hashsearch.c,v
> retrieving revision 1.45
> diff -c -r1.45 hashsearch.c
> *** src/backend/access/hash/hashsearch.c    14 Jul 2006 14:52:17 -0000    1.45
> --- src/backend/access/hash/hashsearch.c    12 Sep 2006 15:44:45 -0000
> ***************
> *** 21,27 ****
>   /*
>    *    _hash_next() -- Get the next item in a scan.
>    *
> !  *        On entry, we have a valid currentItemData in the scan, and a
>    *        pin and read lock on the page that contains that item.
>    *        We find the next item in the scan, if any.
>    *        On success exit, we have the page containing the next item
> --- 21,27 ----
>   /*
>    *    _hash_next() -- Get the next item in a scan.
>    *
> !  *        On entry, we have a valid hashso_curpos in the scan, and a
>    *        pin and read lock on the page that contains that item.
>    *        We find the next item in the scan, if any.
>    *        On success exit, we have the page containing the next item
> ***************
> *** 49,55 ****
>           return false;
>
>       /* if we're here, _hash_step found a valid tuple */
> !     current = &(scan->currentItemData);
>       offnum = ItemPointerGetOffsetNumber(current);
>       _hash_checkpage(rel, buf, LH_BUCKET_PAGE | LH_OVERFLOW_PAGE);
>       page = BufferGetPage(buf);
> --- 49,55 ----
>           return false;
>
>       /* if we're here, _hash_step found a valid tuple */
> !     current = &(so->hashso_curpos);
>       offnum = ItemPointerGetOffsetNumber(current);
>       _hash_checkpage(rel, buf, LH_BUCKET_PAGE | LH_OVERFLOW_PAGE);
>       page = BufferGetPage(buf);
> ***************
> *** 129,135 ****
>
>       pgstat_count_index_scan(&scan->xs_pgstat_info);
>
> !     current = &(scan->currentItemData);
>       ItemPointerSetInvalid(current);
>
>       /*
> --- 129,135 ----
>
>       pgstat_count_index_scan(&scan->xs_pgstat_info);
>
> !     current = &(so->hashso_curpos);
>       ItemPointerSetInvalid(current);
>
>       /*
> ***************
> *** 224,230 ****
>    *    _hash_step() -- step to the next valid item in a scan in the bucket.
>    *
>    *        If no valid record exists in the requested direction, return
> !  *        false.    Else, return true and set the CurrentItemData for the
>    *        scan to the right thing.
>    *
>    *        'bufP' points to the current buffer, which is pinned and read-locked.
> --- 224,230 ----
>    *    _hash_step() -- step to the next valid item in a scan in the bucket.
>    *
>    *        If no valid record exists in the requested direction, return
> !  *        false.    Else, return true and set the hashso_curpos for the
>    *        scan to the right thing.
>    *
>    *        'bufP' points to the current buffer, which is pinned and read-locked.
> ***************
> *** 245,251 ****
>       BlockNumber blkno;
>       IndexTuple    itup;
>
> !     current = &(scan->currentItemData);
>
>       buf = *bufP;
>       _hash_checkpage(rel, buf, LH_BUCKET_PAGE | LH_OVERFLOW_PAGE);
> --- 245,251 ----
>       BlockNumber blkno;
>       IndexTuple    itup;
>
> !     current = &(so->hashso_curpos);
>
>       buf = *bufP;
>       _hash_checkpage(rel, buf, LH_BUCKET_PAGE | LH_OVERFLOW_PAGE);
> Index: src/backend/access/index/genam.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/index/genam.c,v
> retrieving revision 1.58
> diff -c -r1.58 genam.c
> *** src/backend/access/index/genam.c    31 Jul 2006 20:08:59 -0000    1.58
> --- src/backend/access/index/genam.c    12 Sep 2006 15:45:54 -0000
> ***************
> *** 92,100 ****
>
>       scan->opaque = NULL;
>
> -     ItemPointerSetInvalid(&scan->currentItemData);
> -     ItemPointerSetInvalid(&scan->currentMarkData);
> -
>       ItemPointerSetInvalid(&scan->xs_ctup.t_self);
>       scan->xs_ctup.t_data = NULL;
>       scan->xs_cbuf = InvalidBuffer;
> --- 92,97 ----
> Index: src/include/access/gist_private.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/gist_private.h,v
> retrieving revision 1.23
> diff -c -r1.23 gist_private.h
> *** src/include/access/gist_private.h    7 Aug 2006 16:57:57 -0000    1.23
> --- src/include/access/gist_private.h    12 Sep 2006 15:36:30 -0000
> ***************
> *** 72,78 ****
> --- 72,80 ----
>       GISTSTATE  *giststate;
>       MemoryContext tempCxt;
>       Buffer        curbuf;
> +     ItemPointerData curpos;
>       Buffer        markbuf;
> +     ItemPointerData markpos;
>   } GISTScanOpaqueData;
>
>   typedef GISTScanOpaqueData *GISTScanOpaque;
> Index: src/include/access/hash.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/hash.h,v
> retrieving revision 1.73
> diff -c -r1.73 hash.h
> *** src/include/access/hash.h    13 Jul 2006 16:49:19 -0000    1.73
> --- src/include/access/hash.h    12 Sep 2006 15:39:28 -0000
> ***************
> *** 97,102 ****
> --- 97,106 ----
>        */
>       Buffer        hashso_curbuf;
>       Buffer        hashso_mrkbuf;
> +
> +     /* Current and marked position of the scan */
> +     ItemPointerData hashso_curpos;
> +     ItemPointerData hashso_mrkpos;
>   } HashScanOpaqueData;
>
>   typedef HashScanOpaqueData *HashScanOpaque;
> Index: src/include/access/nbtree.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/nbtree.h,v
> retrieving revision 1.104
> diff -c -r1.104 nbtree.h
> *** src/include/access/nbtree.h    24 Aug 2006 01:18:34 -0000    1.104
> --- src/include/access/nbtree.h    12 Sep 2006 15:55:17 -0000
> ***************
> *** 383,391 ****
>    * items were killed, we re-lock the page to mark them killed, then unlock.
>    * Finally we drop the pin and step to the next page in the appropriate
>    * direction.
> -  *
> -  * NOTE: in this implementation, btree does not use or set the
> -  * currentItemData and currentMarkData fields of IndexScanDesc at all.
>    */
>
>   typedef struct BTScanPosItem    /* what we remember about each match */
> --- 383,388 ----
> Index: src/include/access/relscan.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/relscan.h,v
> retrieving revision 1.49
> diff -c -r1.49 relscan.h
> *** src/include/access/relscan.h    31 Jul 2006 20:09:05 -0000    1.49
> --- src/include/access/relscan.h    12 Sep 2006 15:52:50 -0000
> ***************
> *** 69,77 ****
>
>       /* index access method's private state */
>       void       *opaque;            /* access-method-specific info */
> -     /* these fields are used by some but not all AMs: */
> -     ItemPointerData currentItemData;    /* current index pointer */
> -     ItemPointerData currentMarkData;    /* marked position, if any */
>
>       /*
>        * xs_ctup/xs_cbuf are valid after a successful index_getnext. After
> --- 69,74 ----

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly

--
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +