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

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

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

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

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.

I did. Regression tests run fine without it.

-- 
  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:

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.

I did. Regression tests run fine without it.

He asked if you ran the contrib/intarray regress tests, not the
regular ones. (The other GIST contrib modules would also need
to be tested.)

regards, tom lane