Re: [SQL] A bug in gistPageAddItem()/gist_tuple_replacekey()

Here is a good example of why keeping old code around causes confusion.
I encourage the GIST guys to remove the stuff they don't feel they will
ever need. I know Tom may disagree. ;-)

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

Teodor Sigaev wrote:
> gistPageAddItem and gist_tuple_replacekey are commented out by GIST_PAGEADDITEM.
> They was used up to 7.1, but now it is unusable.
>
> gistPageAddItem has interesting feature: recompress entry before writing to
> disk, but we (with Oleg and Tom) couldn't find any reasons to do it. And so, we
> leave this code for later thinking about.
>
> Now gistPageAddItem is wrong, because it can work only in single-key indexes. In
> 7.2 GiST supports multy-key index.
>
>
>
> > I haven't see any comment on this. If no one replies, would you send
> > over a patch of fixes? Thanks.
> >
> > ---------------------------------------------------------------------------
> >
> > Dmitry Tkach wrote:
> >
> >>I was trying to write a gist index extension, and, after some debugging,
> >>it looks like I found a bug somewhere in the gist.c code ...
> >>I can't be quite sure, because I am not familiar with the postgres
> >>code... but, here is what I see happenning (this is 7.1, but I compared
> >>the sources to 7.2, and did not see this fixed - although, I did not
> >>inspect it too carefully)...
> >>
> >>First of all, gistPageAddItem () calls gistdentryinit() with a pointer
> >>to what's stored in the tuple, so, 'by-value' types do not work (because
> >>gistcentryinit () would be passed the value itself, when called from
> >>gistinsert(), and then, in gistPageAddItem (), it is passed a pointer,
> >>coming from gistdentryinit () - so, it just doesn't know really how to
> >>treat the argument)...
> >>
> >>Secondly, gist_tuple_replacekey() seems to have incorrect logic figuring
> >>out if there is enough space in the tuple (it checks for '<', instead of
> >>'<=') - this causes a new tuple to get always created (this one, seems
> >>to be fixed in 7.2)
> >>
> >>Thirdly, gist_tuple_replace_key () sends a pointer to entry.pred (which
> >>is already a pointer to the actual value) to index_formtuple (), that
> >>looks at the tuple, sees that the type is 'pass-by-value', and puts that
> >>pointer directly into the tuple, so that, the resulting tuple now
> >>contains a pointer to a pointer to the actual value...
> >>
> >>Now, if more then one split is required, this sequence is repeated again
> >>and again and again, so that, by the time the tuple gets actually
> >>written, it contains something like a pointer to a pointer to a pointer
> >>to a pointer to the actual data :-(
> >>
> >>Once again, I've seen some comments in the 7.2 branch about gists and
> >>pass-by-value types, but brief looking at the differences in the source
> >>did not make me conveinced that it was indeed fixed...
> >>
> >>Anyone knows otherwise?
> >>
> >>Thanks a lot!
> >>
> >>Dima
> >>
> >>
> >>---------------------------(end of broadcast)---------------------------
> >>TIP 4: Don't 'kill -9' the postmaster
> >>
> >>
> >
>
>
> --
> Teodor Sigaev
> teodor(at)stack(dot)net
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>

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