A bug in gistPageAddItem()/gist_tuple_replacekey() ???
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
[ Cc to hackers.]
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
--
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
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
Import Notes
Reference msg id not found: Pine.GSO.4.44.0204180838420.18506-100000@ra.sai.msu.su | Resolved by subject fallback
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, Pennsylvania 19026