Re: [SQL] A bug in gistPageAddItem()/gist_tuple_replacekey() - Mailing list pgsql-hackers

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@stack.net
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
> 

--  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,
Pennsylvania19026