Обсуждение: Re: GIST question

Do you have any idea why you used the column in the GIST code?  That
code never gets run because the value is never set.

> Bruce,
>
> we dont' see any problem with removing this field (haskeytype)
> but it would be **very desirable** first to run regression test
> (make installcheck) in contrib/intarray after patch applying.
>
>     Regards,
>
>         Oleg
> On Tue, 15 May 2001, Bruce Momjian wrote:
>
> > This GIST question appeared on hackers today.
> >
> >
>
>     Regards,
>         Oleg
> _____________________________________________________________
> Oleg Bartunov, sci.researcher, hostmaster of AstroNet,
> Sternberg Astronomical Institute, Moscow University (Russia)
> Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
> phone: +007(095)939-16-83, +007(095)939-23-83
>
>
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Do you have any idea why you used the column in the GIST code?

Its use in GiST is ancient ... Oleg didn't add it.

            regards, tom lane

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Do you have any idea why you used the column in the GIST code?
>
> Its use in GiST is ancient ... Oleg didn't add it.

OK, I propose the following patch to remove pg_index.indhaskeytype.

The value was always TRUE, so the GIST tests are removed so the code is
always run.  Regression runs fine.

I will keep the patch for a day and apply it if no one objects.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
Index: src/backend/access/gist/gist.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/access/gist/gist.c,v
retrieving revision 1.76
diff -c -r1.76 gist.c
*** src/backend/access/gist/gist.c    2001/05/15 14:14:49    1.76
--- src/backend/access/gist/gist.c    2001/05/15 21:44:39
***************
*** 1127,1151 ****
          elog(ERROR, "initGISTstate: index %u not found",
               RelationGetRelid(index));
      itupform = (Form_pg_index) GETSTRUCT(htup);
-     giststate->haskeytype = itupform->indhaskeytype;
      indexrelid = itupform->indexrelid;
      ReleaseSysCache(htup);

!     if (giststate->haskeytype)
!     {
!         /* key type is different -- is it byval? */
!         htup = SearchSysCache(ATTNUM,
!                               ObjectIdGetDatum(indexrelid),
!                               UInt16GetDatum(FirstOffsetNumber),
!                               0, 0);
!         if (!HeapTupleIsValid(htup))
!             elog(ERROR, "initGISTstate: no attribute tuple %u %d",
!                  indexrelid, FirstOffsetNumber);
!         giststate->keytypbyval = (((Form_pg_attribute) htup)->attbyval);
!         ReleaseSysCache(htup);
!     }
!     else
!         giststate->keytypbyval = FALSE;
  }


--- 1127,1145 ----
          elog(ERROR, "initGISTstate: index %u not found",
               RelationGetRelid(index));
      itupform = (Form_pg_index) GETSTRUCT(htup);
      indexrelid = itupform->indexrelid;
      ReleaseSysCache(htup);

!     /* Is it byval? */
!     htup = SearchSysCache(ATTNUM,
!                           ObjectIdGetDatum(indexrelid),
!                           UInt16GetDatum(FirstOffsetNumber),
!                           0, 0);
!     if (!HeapTupleIsValid(htup))
!         elog(ERROR, "initGISTstate: no attribute tuple %u %d",
!              indexrelid, FirstOffsetNumber);
!     giststate->keytypbyval = (((Form_pg_attribute) htup)->attbyval);
!     ReleaseSysCache(htup);
  }


***************
*** 1197,1215 ****
      GISTENTRY  *dep;

      gistentryinit(*e, pr, r, pg, o, b, l);
!     if (giststate->haskeytype)
!     {
!         if ( b ) {
!             dep = (GISTENTRY *)
!                 DatumGetPointer(FunctionCall1(&giststate->decompressFn,
!                                           PointerGetDatum(e)));
!             gistentryinit(*e, dep->pred, dep->rel, dep->page, dep->offset, dep->bytes,
!                       dep->leafkey);
!             if (dep != e)
!                 pfree(dep);
!         } else {
!             gistentryinit(*e, (char*)NULL, r, pg, o, 0, l);
!         }
      }
  }

--- 1191,1206 ----
      GISTENTRY  *dep;

      gistentryinit(*e, pr, r, pg, o, b, l);
!     if ( b ) {
!         dep = (GISTENTRY *)
!             DatumGetPointer(FunctionCall1(&giststate->decompressFn,
!                                       PointerGetDatum(e)));
!         gistentryinit(*e, dep->pred, dep->rel, dep->page, dep->offset, dep->bytes,
!                   dep->leafkey);
!         if (dep != e)
!             pfree(dep);
!     } else {
!         gistentryinit(*e, (char*)NULL, r, pg, o, 0, l);
      }
  }

***************
*** 1224,1239 ****
      GISTENTRY  *cep;

      gistentryinit(*e, pr, r, pg, o, b, l);
!     if (giststate->haskeytype)
!     {
!         cep = (GISTENTRY *)
!             DatumGetPointer(FunctionCall1(&giststate->compressFn,
!                                           PointerGetDatum(e)));
!         gistentryinit(*e, cep->pred, cep->rel, cep->page, cep->offset, cep->bytes,
!                       cep->leafkey);
!         if (cep != e)
!             pfree(cep);
!     }
  }

  #ifdef GISTDEBUG
--- 1215,1227 ----
      GISTENTRY  *cep;

      gistentryinit(*e, pr, r, pg, o, b, l);
!     cep = (GISTENTRY *)
!         DatumGetPointer(FunctionCall1(&giststate->compressFn,
!                                       PointerGetDatum(e)));
!     gistentryinit(*e, cep->pred, cep->rel, cep->page, cep->offset, cep->bytes,
!                   cep->leafkey);
!     if (cep != e)
!         pfree(cep);
  }

  #ifdef GISTDEBUG
Index: src/backend/catalog/index.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.149
diff -c -r1.149 index.c
*** src/backend/catalog/index.c    2001/05/15 03:49:34    1.149
--- src/backend/catalog/index.c    2001/05/15 21:44:44
***************
*** 589,595 ****
      indexForm->indproc = indexInfo->ii_FuncOid;
      indexForm->indisclustered = false;            /* not used */
      indexForm->indislossy = islossy;
-     indexForm->indhaskeytype = true;            /* used by GIST */
      indexForm->indisunique = indexInfo->ii_Unique;
      indexForm->indisprimary = primary;
      memcpy((char *) &indexForm->indpred, (char *) predText, predLen);
--- 589,594 ----
Index: src/include/access/gist.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/access/gist.h,v
retrieving revision 1.26
diff -c -r1.26 gist.h
*** src/include/access/gist.h    2001/03/22 04:00:26    1.26
--- src/include/access/gist.h    2001/05/15 21:44:46
***************
*** 74,80 ****
      FmgrInfo    penaltyFn;
      FmgrInfo    picksplitFn;
      FmgrInfo    equalFn;
-     bool        haskeytype;
      bool        keytypbyval;
  } GISTSTATE;

--- 74,79 ----
Index: src/include/catalog/catversion.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/catalog/catversion.h,v
retrieving revision 1.78
diff -c -r1.78 catversion.h
*** src/include/catalog/catversion.h    2001/05/15 03:49:35    1.78
--- src/include/catalog/catversion.h    2001/05/15 21:44:46
***************
*** 53,58 ****
   */

  /*                            yyyymmddN */
! #define CATALOG_VERSION_NO    200105145

  #endif
--- 53,58 ----
   */

  /*                            yyyymmddN */
! #define CATALOG_VERSION_NO    200105151

  #endif
Index: src/include/catalog/pg_index.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/catalog/pg_index.h,v
retrieving revision 1.19
diff -c -r1.19 pg_index.h
*** src/include/catalog/pg_index.h    2001/05/15 03:49:35    1.19
--- src/include/catalog/pg_index.h    2001/05/15 21:44:46
***************
*** 37,44 ****
  /*
   * it seems that all variable length fields should go at the _end_,
   * because the system cache routines only copy the fields up to the
!  * first variable length field.  so I moved indislossy, indhaskeytype,
!  * and indisunique before indpred.    --djm 8/20/96
   */
  CATALOG(pg_index)
  {
--- 37,44 ----
  /*
   * it seems that all variable length fields should go at the _end_,
   * because the system cache routines only copy the fields up to the
!  * first variable length field.  so I moved indislossy and indisunique
!  * before indpred.    --djm 8/20/96
   */
  CATALOG(pg_index)
  {
***************
*** 54,60 ****
                                   */
      bool        indislossy;        /* do we fetch false tuples (lossy
                                   * compression)? */
-     bool        indhaskeytype;    /* does key type != attribute type? */
      bool        indisunique;    /* is this a unique index? */
      bool        indisprimary;    /* is this index for primary key */
      Oid            indreference;    /* oid of index of referenced relation (ie
--- 54,59 ----
***************
*** 73,79 ****
   *        compiler constants for pg_index
   * ----------------
   */
! #define Natts_pg_index                    12
  #define Anum_pg_index_indexrelid        1
  #define Anum_pg_index_indrelid            2
  #define Anum_pg_index_indproc            3
--- 72,78 ----
   *        compiler constants for pg_index
   * ----------------
   */
! #define Natts_pg_index                    11
  #define Anum_pg_index_indexrelid        1
  #define Anum_pg_index_indrelid            2
  #define Anum_pg_index_indproc            3
***************
*** 81,90 ****
  #define Anum_pg_index_indclass            5
  #define Anum_pg_index_indisclustered    6
  #define Anum_pg_index_indislossy        7
! #define Anum_pg_index_indhaskeytype        8
! #define Anum_pg_index_indisunique        9
! #define Anum_pg_index_indisprimary        10
! #define Anum_pg_index_indreference        11
! #define Anum_pg_index_indpred            12

  #endif     /* PG_INDEX_H */
--- 80,88 ----
  #define Anum_pg_index_indclass            5
  #define Anum_pg_index_indisclustered    6
  #define Anum_pg_index_indislossy        7
! #define Anum_pg_index_indisunique        8
! #define Anum_pg_index_indisprimary        9
! #define Anum_pg_index_indreference        10
! #define Anum_pg_index_indpred            11

  #endif     /* PG_INDEX_H */
Index: src/test/regress/expected/opr_sanity.out
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/test/regress/expected/opr_sanity.out,v
retrieving revision 1.26
diff -c -r1.26 opr_sanity.out
*** src/test/regress/expected/opr_sanity.out    2001/05/15 04:12:56    1.26
--- src/test/regress/expected/opr_sanity.out    2001/05/15 21:44:53
***************
*** 482,489 ****
            (p2.pronargs = 1 AND p1.aggbasetype = 0)));
    oid  | aggname | oid |   proname
  -------+---------+-----+-------------
!  17009 | max     | 768 | int4larger
!  17023 | min     | 769 | int4smaller
  (2 rows)

  -- Cross-check finalfn (if present) against its entry in pg_proc.
--- 482,489 ----
            (p2.pronargs = 1 AND p1.aggbasetype = 0)));
    oid  | aggname | oid |   proname
  -------+---------+-----+-------------
!  17008 | max     | 768 | int4larger
!  17022 | min     | 769 | int4smaller
  (2 rows)

  -- Cross-check finalfn (if present) against its entry in pg_proc.

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I will keep the patch for a day and apply it if no one objects.

I object.  You still have no idea what that test is for or whether
there may be any value in keeping it.  It seems clear that the original
GIST authors thought the flag was useful.

I should also point out that the fact that the flag is always "true"
today is because I ripped out some code in index.c a version or three
back.  6.5 had

    indexForm->indhaskeytype = 0;
    while (attributeList != NIL)
    {
        IndexKey = (IndexElem *) lfirst(attributeList);
        if (IndexKey->typename != NULL)
        {
            indexForm->indhaskeytype = 1;
            break;
        }
        attributeList = lnext(attributeList);
    }

which I removed because it was a security hole (you could tell the
system to treat any data type as any other datatype, with obvious
possibilities for coredump).  But I didn't look hard at what the
GIST code was using the flag for...

            regards, tom lane

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I will keep the patch for a day and apply it if no one objects.
>
> I object.  You still have no idea what that test is for or whether
> there may be any value in keeping it.  It seems clear that the original
> GIST authors thought the flag was useful.
>
> I should also point out that the fact that the flag is always "true"
> today is because I ripped out some code in index.c a version or three
> back.  6.5 had
>
>     indexForm->indhaskeytype = 0;
>     while (attributeList != NIL)
>     {
>         IndexKey = (IndexElem *) lfirst(attributeList);
>         if (IndexKey->typename != NULL)
>         {
>             indexForm->indhaskeytype = 1;
>             break;
>         }
>         attributeList = lnext(attributeList);
>     }
>
> which I removed because it was a security hole (you could tell the
> system to treat any data type as any other datatype, with obvious
> possibilities for coredump).  But I didn't look hard at what the
> GIST code was using the flag for...

I did look at the code inside the tests.  The first was to decide if it
was suppose to look in pg_attribute for the byvalue flag.  The last two
controlled the if gistentryinit() was called.  No idea what that is
because the variable names are single letters.  Gistentryinit() is:

#define gistentryinit(e, pr, r, pg, o, b, l)\
  do {(e).pred = pr; (e).rel = r; (e).page = pg; (e).offset = o; (e).bytes = b;
 (e).leafkey = l;} while (0)

I have to say I have no idea how pg_index.indhaskeytype is related to
gistentryinit().

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

> I did look at the code inside the tests.  The first was to decide if it
> was suppose to look in pg_attribute for the byvalue flag.  The last two
> controlled the if gistentryinit() was called.  No idea what that is
> because the variable names are single letters.  Gistentryinit() is:
>
> #define gistentryinit(e, pr, r, pg, o, b, l)\
>   do {(e).pred = pr; (e).rel = r; (e).page = pg; (e).offset = o; (e).bytes = b;
>  (e).leafkey = l;} while (0)
>
> I have to say I have no idea how pg_index.indhaskeytype is related to
> gistentryinit().

I figured out part of it.   gistentryinit() is related to compressFn and
decompressFn in its calls.  It seems they wanted to disable those calls
when haskeytype was false.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I will keep the patch for a day and apply it if no one objects.
>
> I object.  You still have no idea what that test is for or whether
> there may be any value in keeping it.  It seems clear that the original
> GIST authors thought the flag was useful.
>
> I should also point out that the fact that the flag is always "true"
> today is because I ripped out some code in index.c a version or three
> back.  6.5 had
>
>     indexForm->indhaskeytype = 0;
>     while (attributeList != NIL)
>     {
>         IndexKey = (IndexElem *) lfirst(attributeList);
>         if (IndexKey->typename != NULL)
>         {
>             indexForm->indhaskeytype = 1;
>             break;
>         }
>         attributeList = lnext(attributeList);
>     }
>
> which I removed because it was a security hole (you could tell the
> system to treat any data type as any other datatype, with obvious
> possibilities for coredump).  But I didn't look hard at what the
> GIST code was using the flag for...

OK, I think it makes sense now.  When you over-rode the type, it would
not use the system tables for byvalue and for compression/decompression.

Now, Tom, if you disabled such over-riding, seems the tests are useless
now, right?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

On Tue, 15 May 2001, Bruce Momjian wrote:

>
> Do you have any idea why you used the column in the GIST code?  That
> code never gets run because the value is never set.
>

We have no idea. This piece of code was written before us.

    Oleg

> > Bruce,
> >
> > we dont' see any problem with removing this field (haskeytype)
> > but it would be **very desirable** first to run regression test
> > (make installcheck) in contrib/intarray after patch applying.
> >
> >     Regards,
> >
> >         Oleg
> > On Tue, 15 May 2001, Bruce Momjian wrote:
> >
> > > This GIST question appeared on hackers today.
> > >
> > >
> >
> >     Regards,
> >         Oleg
> > _____________________________________________________________
> > Oleg Bartunov, sci.researcher, hostmaster of AstroNet,
> > Sternberg Astronomical Institute, Moscow University (Russia)
> > Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
> > phone: +007(095)939-16-83, +007(095)939-23-83
> >
> >
> >
>
>

    Regards,
        Oleg
_____________________________________________________________
Oleg Bartunov, sci.researcher, hostmaster of AstroNet,
Sternberg Astronomical Institute, Moscow University (Russia)
Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
phone: +007(095)939-16-83, +007(095)939-23-83