Re: [HACKERS] Unusable SP-GiST index

I wrote:
> After looking a bit at gist and sp-gist, neither of them would find that
> terribly convenient; they really want to create one blob of memory per
> index entry so as to not complicate storage management too much.  But
> they'd be fine with making that blob be a HeapTuple not IndexTuple.
> So maybe the right approach is to expand the existing API to allow the
> AM to return *either* a heap or index tuple; that could be made to not
> be an API break.

Here's a draft patch along those lines.  With this approach, btree doesn't
need to be touched at all, since what it's returning certainly is an
IndexTuple anyway.  I fixed both SPGIST and GIST to use HeapTuple return
format.  It's not very clear to me whether GIST has a similar hazard with
very large return values, but it might, and it's simple enough to change.

            regards, tom lane

diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 40f201b..d4af010 100644
*** a/doc/src/sgml/indexam.sgml
--- b/doc/src/sgml/indexam.sgml
*************** amgettuple (IndexScanDesc scan,
*** 535,549 ****
    <para>
     If the index supports <link linkend="indexes-index-only-scans">index-only
     scans</link> (i.e., <function>amcanreturn</function> returns TRUE for it),
!    then on success the AM must also check
!    <literal>scan->xs_want_itup</>, and if that is true it must return
!    the original indexed data for the index entry, in the form of an
     <structname>IndexTuple</> pointer stored at <literal>scan->xs_itup</>,
!    with tuple descriptor <literal>scan->xs_itupdesc</>.
!    (Management of the data referenced by the pointer is the access method's
     responsibility.  The data must remain good at least until the next
     <function>amgettuple</>, <function>amrescan</>, or <function>amendscan</>
!    call for the scan.)
    </para>

    <para>
--- 535,553 ----
    <para>
     If the index supports <link linkend="indexes-index-only-scans">index-only
     scans</link> (i.e., <function>amcanreturn</function> returns TRUE for it),
!    then on success the AM must also check <literal>scan->xs_want_itup</>,
!    and if that is true it must return the originally indexed data for the
!    index entry.  The data can be returned in the form of an
     <structname>IndexTuple</> pointer stored at <literal>scan->xs_itup</>,
!    with tuple descriptor <literal>scan->xs_itupdesc</>; or in the form of
!    a <structname>HeapTuple</> pointer stored at <literal>scan->xs_hitup</>,
!    with tuple descriptor <literal>scan->xs_hitupdesc</>.  (The latter
!    format should be used when reconstructing data that might possibly not fit
!    into an IndexTuple.)  In either case,
!    management of the data referenced by the pointer is the access method's
     responsibility.  The data must remain good at least until the next
     <function>amgettuple</>, <function>amrescan</>, or <function>amendscan</>
!    call for the scan.
    </para>

    <para>
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index eea366b..122dc38 100644
*** a/src/backend/access/gist/gistget.c
--- b/src/backend/access/gist/gistget.c
*************** gistScanPage(IndexScanDesc scan, GISTSea
*** 441,452 ****
              so->pageData[so->nPageData].offnum = i;

              /*
!              * In an index-only scan, also fetch the data from the tuple.
               */
              if (scan->xs_want_itup)
              {
                  oldcxt = MemoryContextSwitchTo(so->pageDataCxt);
!                 so->pageData[so->nPageData].ftup =
                      gistFetchTuple(giststate, r, it);
                  MemoryContextSwitchTo(oldcxt);
              }
--- 441,453 ----
              so->pageData[so->nPageData].offnum = i;

              /*
!              * In an index-only scan, also fetch the data from the tuple.  The
!              * reconstructed tuples are stored in pageDataCxt.
               */
              if (scan->xs_want_itup)
              {
                  oldcxt = MemoryContextSwitchTo(so->pageDataCxt);
!                 so->pageData[so->nPageData].recontup =
                      gistFetchTuple(giststate, r, it);
                  MemoryContextSwitchTo(oldcxt);
              }
*************** gistScanPage(IndexScanDesc scan, GISTSea
*** 478,484 ****
                   * In an index-only scan, also fetch the data from the tuple.
                   */
                  if (scan->xs_want_itup)
!                     item->data.heap.ftup = gistFetchTuple(giststate, r, it);
              }
              else
              {
--- 479,485 ----
                   * In an index-only scan, also fetch the data from the tuple.
                   */
                  if (scan->xs_want_itup)
!                     item->data.heap.recontup = gistFetchTuple(giststate, r, it);
              }
              else
              {
*************** getNextNearest(IndexScanDesc scan)
*** 540,550 ****
      bool        res = false;
      int            i;

!     if (scan->xs_itup)
      {
          /* free previously returned tuple */
!         pfree(scan->xs_itup);
!         scan->xs_itup = NULL;
      }

      do
--- 541,551 ----
      bool        res = false;
      int            i;

!     if (scan->xs_hitup)
      {
          /* free previously returned tuple */
!         pfree(scan->xs_hitup);
!         scan->xs_hitup = NULL;
      }

      do
*************** getNextNearest(IndexScanDesc scan)
*** 601,607 ****

              /* in an index-only scan, also return the reconstructed tuple. */
              if (scan->xs_want_itup)
!                 scan->xs_itup = item->data.heap.ftup;
              res = true;
          }
          else
--- 602,608 ----

              /* in an index-only scan, also return the reconstructed tuple. */
              if (scan->xs_want_itup)
!                 scan->xs_hitup = item->data.heap.recontup;
              res = true;
          }
          else
*************** gistgettuple(IndexScanDesc scan, ScanDir
*** 685,691 ****

                  /* in an index-only scan, also return the reconstructed tuple */
                  if (scan->xs_want_itup)
!                     scan->xs_itup = so->pageData[so->curPageData].ftup;

                  so->curPageData++;

--- 686,692 ----

                  /* in an index-only scan, also return the reconstructed tuple */
                  if (scan->xs_want_itup)
!                     scan->xs_hitup = so->pageData[so->curPageData].recontup;

                  so->curPageData++;

diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 33b3889..81ff8fc 100644
*** a/src/backend/access/gist/gistscan.c
--- b/src/backend/access/gist/gistscan.c
*************** gistrescan(IndexScanDesc scan, ScanKey k
*** 155,161 ****
       * tuple descriptor to represent the returned index tuples and create a
       * memory context to hold them during the scan.
       */
!     if (scan->xs_want_itup && !scan->xs_itupdesc)
      {
          int            natts;
          int            attno;
--- 155,161 ----
       * tuple descriptor to represent the returned index tuples and create a
       * memory context to hold them during the scan.
       */
!     if (scan->xs_want_itup && !scan->xs_hitupdesc)
      {
          int            natts;
          int            attno;
*************** gistrescan(IndexScanDesc scan, ScanKey k
*** 174,181 ****
                                 scan->indexRelation->rd_opcintype[attno - 1],
                                 -1, 0);
          }
!         scan->xs_itupdesc = so->giststate->fetchTupdesc;

          so->pageDataCxt = AllocSetContextCreate(so->giststate->scanCxt,
                                                  "GiST page data context",
                                                  ALLOCSET_DEFAULT_SIZES);
--- 174,182 ----
                                 scan->indexRelation->rd_opcintype[attno - 1],
                                 -1, 0);
          }
!         scan->xs_hitupdesc = so->giststate->fetchTupdesc;

+         /* Also create a memory context that will hold the returned tuples */
          so->pageDataCxt = AllocSetContextCreate(so->giststate->scanCxt,
                                                  "GiST page data context",
                                                  ALLOCSET_DEFAULT_SIZES);
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index f92baed..75845ba 100644
*** a/src/backend/access/gist/gistutil.c
--- b/src/backend/access/gist/gistutil.c
*************** gistFetchAtt(GISTSTATE *giststate, int n
*** 624,632 ****

  /*
   * Fetch all keys in tuple.
!  * returns new IndexTuple that contains GISTENTRY with fetched data
   */
! IndexTuple
  gistFetchTuple(GISTSTATE *giststate, Relation r, IndexTuple tuple)
  {
      MemoryContext oldcxt = MemoryContextSwitchTo(giststate->tempCxt);
--- 624,632 ----

  /*
   * Fetch all keys in tuple.
!  * Returns a new HeapTuple containing the originally-indexed data.
   */
! HeapTuple
  gistFetchTuple(GISTSTATE *giststate, Relation r, IndexTuple tuple)
  {
      MemoryContext oldcxt = MemoryContextSwitchTo(giststate->tempCxt);
*************** gistFetchTuple(GISTSTATE *giststate, Rel
*** 660,666 ****
      }
      MemoryContextSwitchTo(oldcxt);

!     return index_form_tuple(giststate->fetchTupdesc, fetchatt, isnull);
  }

  float
--- 660,666 ----
      }
      MemoryContextSwitchTo(oldcxt);

!     return heap_form_tuple(giststate->fetchTupdesc, fetchatt, isnull);
  }

  float
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index c4a393f..3599476 100644
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
*************** RelationGetIndexScan(Relation indexRelat
*** 119,124 ****
--- 119,126 ----

      scan->xs_itup = NULL;
      scan->xs_itupdesc = NULL;
+     scan->xs_hitup = NULL;
+     scan->xs_hitupdesc = NULL;

      ItemPointerSetInvalid(&scan->xs_ctup.t_self);
      scan->xs_ctup.t_data = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 4822af9..89d1971 100644
*** a/src/backend/access/index/indexam.c
--- b/src/backend/access/index/indexam.c
*************** index_getnext_tid(IndexScanDesc scan, Sc
*** 409,416 ****
      /*
       * The AM's amgettuple proc finds the next index entry matching the scan
       * keys, and puts the TID into scan->xs_ctup.t_self.  It should also set
!      * scan->xs_recheck and possibly scan->xs_itup, though we pay no attention
!      * to those fields here.
       */
      found = scan->indexRelation->rd_amroutine->amgettuple(scan, direction);

--- 409,416 ----
      /*
       * The AM's amgettuple proc finds the next index entry matching the scan
       * keys, and puts the TID into scan->xs_ctup.t_self.  It should also set
!      * scan->xs_recheck and possibly scan->xs_itup/scan->xs_hitup, though we
!      * pay no attention to those fields here.
       */
      found = scan->indexRelation->rd_amroutine->amgettuple(scan, direction);

diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 139d998..2d96c00 100644
*** a/src/backend/access/spgist/spgscan.c
--- b/src/backend/access/spgist/spgscan.c
*************** resetSpGistScanOpaque(SpGistScanOpaque s
*** 92,102 ****

      if (so->want_itup)
      {
!         /* Must pfree IndexTuples to avoid memory leak */
          int            i;

          for (i = 0; i < so->nPtrs; i++)
!             pfree(so->indexTups[i]);
      }
      so->iPtr = so->nPtrs = 0;
  }
--- 92,102 ----

      if (so->want_itup)
      {
!         /* Must pfree reconstructed tuples to avoid memory leak */
          int            i;

          for (i = 0; i < so->nPtrs; i++)
!             pfree(so->reconTups[i]);
      }
      so->iPtr = so->nPtrs = 0;
  }
*************** spgbeginscan(Relation rel, int keysz, in
*** 195,202 ****
                                          "SP-GiST search temporary context",
                                          ALLOCSET_DEFAULT_SIZES);

!     /* Set up indexTupDesc and xs_itupdesc in case it's an index-only scan */
!     so->indexTupDesc = scan->xs_itupdesc = RelationGetDescr(rel);

      scan->opaque = so;

--- 195,202 ----
                                          "SP-GiST search temporary context",
                                          ALLOCSET_DEFAULT_SIZES);

!     /* Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan */
!     so->indexTupDesc = scan->xs_hitupdesc = RelationGetDescr(rel);

      scan->opaque = so;

*************** storeGettuple(SpGistScanOpaque so, ItemP
*** 591,602 ****
      if (so->want_itup)
      {
          /*
!          * Reconstruct desired IndexTuple.  We have to copy the datum out of
!          * the temp context anyway, so we may as well create the tuple here.
           */
!         so->indexTups[so->nPtrs] = index_form_tuple(so->indexTupDesc,
!                                                     &leafValue,
!                                                     &isnull);
      }
      so->nPtrs++;
  }
--- 591,602 ----
      if (so->want_itup)
      {
          /*
!          * Reconstruct index data.  We have to copy the datum out of the temp
!          * context anyway, so we may as well create the tuple here.
           */
!         so->reconTups[so->nPtrs] = heap_form_tuple(so->indexTupDesc,
!                                                    &leafValue,
!                                                    &isnull);
      }
      so->nPtrs++;
  }
*************** spggettuple(IndexScanDesc scan, ScanDire
*** 619,636 ****
              /* continuing to return tuples from a leaf page */
              scan->xs_ctup.t_self = so->heapPtrs[so->iPtr];
              scan->xs_recheck = so->recheck[so->iPtr];
!             scan->xs_itup = so->indexTups[so->iPtr];
              so->iPtr++;
              return true;
          }

          if (so->want_itup)
          {
!             /* Must pfree IndexTuples to avoid memory leak */
              int            i;

              for (i = 0; i < so->nPtrs; i++)
!                 pfree(so->indexTups[i]);
          }
          so->iPtr = so->nPtrs = 0;

--- 619,636 ----
              /* continuing to return tuples from a leaf page */
              scan->xs_ctup.t_self = so->heapPtrs[so->iPtr];
              scan->xs_recheck = so->recheck[so->iPtr];
!             scan->xs_hitup = so->reconTups[so->iPtr];
              so->iPtr++;
              return true;
          }

          if (so->want_itup)
          {
!             /* Must pfree reconstructed tuples to avoid memory leak */
              int            i;

              for (i = 0; i < so->nPtrs; i++)
!                 pfree(so->reconTups[i]);
          }
          so->iPtr = so->nPtrs = 0;

diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index ddef3a4..62bdeb3 100644
*** a/src/backend/executor/nodeIndexonlyscan.c
--- b/src/backend/executor/nodeIndexonlyscan.c
*************** IndexOnlyNext(IndexOnlyScanState *node)
*** 144,152 ****
          }

          /*
!          * Fill the scan tuple slot with data from the index.
           */
!         StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);

          /*
           * If the index was lossy, we have to recheck the index quals.
--- 144,169 ----
          }

          /*
!          * Fill the scan tuple slot with data from the index.  This might be
!          * provided in either HeapTuple or IndexTuple format.  Conceivably an
!          * index AM might fill both fields, in which case we prefer the heap
!          * format, since it's probably a bit cheaper to fill a slot from.
           */
!         if (scandesc->xs_hitup)
!         {
!             /*
!              * We don't take the trouble to verify that the provided tuple has
!              * exactly the slot's format, but it seems worth doing a quick
!              * check on the number of fields.
!              */
!             Assert(slot->tts_tupleDescriptor->natts ==
!                    scandesc->xs_hitupdesc->natts);
!             ExecStoreTuple(scandesc->xs_hitup, slot, InvalidBuffer, false);
!         }
!         else if (scandesc->xs_itup)
!             StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
!         else
!             elog(ERROR, "no data returned for index-only scan");

          /*
           * If the index was lossy, we have to recheck the index quals.
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 72f52d4..6522336 100644
*** a/src/include/access/gist_private.h
--- b/src/include/access/gist_private.h
*************** typedef struct GISTSearchHeapItem
*** 120,126 ****
      ItemPointerData heapPtr;
      bool        recheck;        /* T if quals must be rechecked */
      bool        recheckDistances;        /* T if distances must be rechecked */
!     IndexTuple    ftup;            /* data fetched back from the index, used in
                                   * index-only scans */
      OffsetNumber offnum;        /* track offset in page to mark tuple as
                                   * LP_DEAD */
--- 120,126 ----
      ItemPointerData heapPtr;
      bool        recheck;        /* T if quals must be rechecked */
      bool        recheckDistances;        /* T if distances must be rechecked */
!     HeapTuple    recontup;        /* data reconstructed from the index, used in
                                   * index-only scans */
      OffsetNumber offnum;        /* track offset in page to mark tuple as
                                   * LP_DEAD */
*************** extern void gistMakeUnionItVec(GISTSTATE
*** 529,535 ****
  extern bool gistKeyIsEQ(GISTSTATE *giststate, int attno, Datum a, Datum b);
  extern void gistDeCompressAtt(GISTSTATE *giststate, Relation r, IndexTuple tuple, Page p,
                    OffsetNumber o, GISTENTRY *attdata, bool *isnull);
! extern IndexTuple gistFetchTuple(GISTSTATE *giststate, Relation r,
                 IndexTuple tuple);
  extern void gistMakeUnionKey(GISTSTATE *giststate, int attno,
                   GISTENTRY *entry1, bool isnull1,
--- 529,535 ----
  extern bool gistKeyIsEQ(GISTSTATE *giststate, int attno, Datum a, Datum b);
  extern void gistDeCompressAtt(GISTSTATE *giststate, Relation r, IndexTuple tuple, Page p,
                    OffsetNumber o, GISTENTRY *attdata, bool *isnull);
! extern HeapTuple gistFetchTuple(GISTSTATE *giststate, Relation r,
                 IndexTuple tuple);
  extern void gistMakeUnionKey(GISTSTATE *giststate, int attno,
                   GISTENTRY *entry1, bool isnull1,
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 8746045..8635f83 100644
*** a/src/include/access/relscan.h
--- b/src/include/access/relscan.h
*************** typedef struct IndexScanDescData
*** 103,111 ****
      /* index access method's private state */
      void       *opaque;            /* access-method-specific info */

!     /* in an index-only scan, this is valid after a successful amgettuple */
      IndexTuple    xs_itup;        /* index tuple returned by AM */
      TupleDesc    xs_itupdesc;    /* rowtype descriptor of xs_itup */

      /* xs_ctup/xs_cbuf/xs_recheck are valid after a successful index_getnext */
      HeapTupleData xs_ctup;        /* current heap tuple, if any */
--- 103,118 ----
      /* index access method's private state */
      void       *opaque;            /* access-method-specific info */

!     /*
!      * In an index-only scan, a successful amgettuple call must fill either
!      * xs_itup (and xs_itupdesc) or xs_hitup (and xs_hitupdesc) to provide the
!      * data returned by the scan.  It can fill both, in which case the heap
!      * format will be used.
!      */
      IndexTuple    xs_itup;        /* index tuple returned by AM */
      TupleDesc    xs_itupdesc;    /* rowtype descriptor of xs_itup */
+     HeapTuple    xs_hitup;        /* index data returned by AM, as HeapTuple */
+     TupleDesc    xs_hitupdesc;    /* rowtype descriptor of xs_hitup */

      /* xs_ctup/xs_cbuf/xs_recheck are valid after a successful index_getnext */
      HeapTupleData xs_ctup;        /* current heap tuple, if any */
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index b2979a9..67d45e8 100644
*** a/src/include/access/spgist_private.h
--- b/src/include/access/spgist_private.h
*************** typedef struct SpGistScanOpaqueData
*** 159,165 ****
      int            iPtr;            /* index for scanning through same */
      ItemPointerData heapPtrs[MaxIndexTuplesPerPage];    /* TIDs from cur page */
      bool        recheck[MaxIndexTuplesPerPage]; /* their recheck flags */
!     IndexTuple    indexTups[MaxIndexTuplesPerPage];        /* reconstructed tuples */

      /*
       * Note: using MaxIndexTuplesPerPage above is a bit hokey since
--- 159,165 ----
      int            iPtr;            /* index for scanning through same */
      ItemPointerData heapPtrs[MaxIndexTuplesPerPage];    /* TIDs from cur page */
      bool        recheck[MaxIndexTuplesPerPage]; /* their recheck flags */
!     HeapTuple    reconTups[MaxIndexTuplesPerPage];        /* reconstructed tuples */

      /*
       * Note: using MaxIndexTuplesPerPage above is a bit hokey since

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers