Thread: GiST on 64-bit box
Hello,
We have report that all GiST opclasses cause a coredump while creating index on
64-bit Solaris. I try to make installcheck GiST's contrib modules on DEC Alpha
and got the same result. So it may be one problem.
Compiler:
gcc -v
Reading specs from
/usr/local/egcs/lib/gcc-lib/alphaev56-dec-osf4.0c/egcs-2.90.23/specs
gcc version egcs-2.90.23 980102 (egcs-1.0.1 release)
Backtrace:
#0 0x12003d564 in gistdentryinit (giststate=0x11fffd820, nkey=0, e=0x1401b25a4,
k=5370611216, r=0x140171ce8, pg=0x1401ce620 "", o=1, b=8, l=0 '\000', isNull=0 '\000') at gist.c:1754
#1 0x12003b3b0 in gistSplit (r=0x140171ce8, buffer=-1, itup=0x140196a10,
len=0x11fffd598, giststate=0x11fffd820, res=0x0) at gist.c:1264
#2 0x120037be4 in gistlayerinsert (r=0x140171ce8, blkno=1075407376,
itup=0x11fffd610, len=0x11fffd618, res=0x0, giststate=0x11fffd820) at gist.c:515
#3 0x1200376d8 in gistdoinsert (r=0x140171ce8, itup=0x140196718, res=0x0,
giststate=0x11fffd820) at gist.c:426
#4 0x120037228 in gistbuildCallback (index=0x140171ce8, htup=0x140196590,
attdata=0x11fffd710, nulls=0x11fffd790 " о\017@\001", tupleIsAlive=-24 'Х', state=0x1401ce620)
at gist.c:275
#5 0x12008c4c4 in IndexBuildHeapScan (heapRelation=0x1,
indexRelation=0x140171ce8, indexInfo=0x140196238, callback=0x120037020 <gistbuildCallback>,
callback_state=0x11fffd820)at
index.c:1805
#6 0x120036f70 in gistbuild (fcinfo=0x11fffd820) at gist.c:186
#7 0x1201df8c4 in OidFunctionCall3 (functionId=536860704, arg1=5370215808,
arg2=5370223848, arg3=5370372664) at fmgr.c:1190
...
Output to sdterr:
Unaligned access pid=6018 <postgres> va=0x1401b25a4 pc=0x12003d560
ra=0x12003b3b0 inst=0xb6690000
Source (gist.c, around 1264 line): /* generate the item array */ entryvec = (bytea *) palloc(VARHDRSZ +
(*len+ 1) * sizeof(GISTENTRY)); decompvec = (bool *) palloc(VARHDRSZ + (*len + 1) * sizeof(bool));
VARATT_SIZEP(entryvec)= (*len + 1) * sizeof(GISTENTRY) + VARHDRSZ; for (i = 1; i <= *len; i++) {
datum = index_getattr(itup[i - 1], 1, giststate->tupdesc, &IsNull); gistdentryinit(giststate, 0,
&((GISTENTRY*) VARDATA(entryvec))[i], datum, r, p, i,
ATTSIZE(datum,giststate->tupdesc, 1, IsNull), FALSE, IsNull); if ((!isAttByVal(giststate, 0)) &&
((GISTENTRY*)
VARDATA(entryvec))[i].key != datum) decompvec[i] = TRUE; else
decompvec[i] = FALSE; }
Core dump causes on first call gistdentryinit, because pointer
&((GISTENTRY *) VARDATA(entryvec))[1] has not 8-byte aligment. Difference
between entryvec and this pointer is equal 44 bytes.
Can you give some advice how make aligment? or, may be exist another way for
solving...
BTW, on HPUX 11.0 all works fine.
Thank you.
--
Teodor Sigaev
teodor@stack.net
Teodor Sigaev <teodor@stack.net> writes:
> We have report that all GiST opclasses cause a coredump while creating
> index on 64-bit Solaris. I try to make installcheck GiST's contrib
> modules on DEC Alpha and got the same result. So it may be one
> problem.
Yes. It looks to me like GIST is broken on any platform that has 8-byte
Datum (or pointer) and requires 8-byte alignment of same.
The problem is that GISTENTRY will require 8-byte alignment on such a
platform, and this code is not honoring that: it's trying to set up an
array of GISTENTRYs starting at offset 4 in a palloc'd memory chunk.
Apparently, the reason for the offset is to stick a varlena header on
the parameter being passed to the picksplit function. This seems like
it might be unnecessary. Is there another way for the picksplit
function to learn the length of the array?
I think you have two possible ways to proceed:
1. Modify the code to use MAXALIGN(VARHDRSZ) rather than just VARHDRSZ
as the offset in the bogus bytea construct. This would be messy since
you couldn't use VARDATA() anymore.
2. Forget the bytea header and just treat the object as a GISTENTRY
array.
Either one of these is going to require changing the picksplit functions
as well as the calling code, so they're both bad choices from a
maintenance point of view. I think I lean towards #2 since it will make
the code less ugly rather than more so.
regards, tom lane
My opinion is that second way is right (use GISTENTRY array). But this channge requires changes in GiST API: picksplit and union functions must retrieve one argument more. Is it possible to make for 7.2.1 or such changes must be appyed in TODO for 7.3 ? Tom Lane wrote: > I think you have two possible ways to proceed: > > 1. Modify the code to use MAXALIGN(VARHDRSZ) rather than just VARHDRSZ > as the offset in the bogus bytea construct. This would be messy since > you couldn't use VARDATA() anymore. > > 2. Forget the bytea header and just treat the object as a GISTENTRY > array. > > Either one of these is going to require changing the picksplit functions > as well as the calling code, so they're both bad choices from a > maintenance point of view. I think I lean towards #2 since it will make > the code less ugly rather than more so. > > regards, tom lane > > -- Teodor Sigaev teodor@stack.net
Teodor Sigaev <teodor@stack.net> writes:
> My opinion is that second way is right (use GISTENTRY array). But this
> channge requires changes in GiST API: picksplit and union functions
> must retrieve one argument more. Is it possible to make for 7.2.1 or
> such changes must be appyed in TODO for 7.3 ?
Hmm. If we had any such functions installed in the standard system,
then such a change would mean an initdb, which we couldn't do for 7.2.1.
We could argue that forcing a change in a contrib module isn't an
initdb, but the argument will seem very thin to anyone who has a
running 7.2 database with GIST indexes and wants to update to 7.2.1.
They will have to reinstall their GIST support modules and recreate
their GIST indexes, AFAICS.
On the other hand, changing the signature would be a good thing if the
GIST code were tweaked to check that the referenced function had the
right signature, because that way you could raise an error at runtime
if someone tried to use a non-updated contrib module with an updated
backend. Without some such check I foresee disasters in the field.
So I think I vote for changing the signature and tweaking initGISTstate
to verify that the number of parameters each function expects is right.
But even with that, it might be argued that we should postpone the
change till 7.3, and just say "sorry folks, GIST doesn't work on
64-bit machines for now". Is that worse than risking update problems
for existing users of GIST indexes? Comments anyone?
regards, tom lane
Actually, there is a third possibility, which would fix the problem
without requiring any changes in the picksplit functions. You could
do this:
char *storage;
storage = palloc(MAXALIGN(VARHDRSZ) + (*len + 1) * sizeof(GISTENTRY)); entryvec = (bytea *) (storage +
MAXALIGN(VARHDRSZ)- VARHDRSZ);
use entryvec as before, except final pfree is pfree(storage)
Grotty as heck, but probably the right answer for 7.2.1 to avoid the
initdb issues.
For 7.3 we could do it the other, cleaner way.
regards, tom lane
On Fri, Feb 08, 2002 at 12:42:18PM -0500, Tom Lane wrote: > Teodor Sigaev <teodor@stack.net> writes: > But even with that, it might be argued that we should postpone the > change till 7.3, and just say "sorry folks, GIST doesn't work on > 64-bit machines for now". Is that worse than risking update problems > for existing users of GIST indexes? Comments anyone? I would change it for 7.3 but have a patch somewhere downloadable for those, who need it ASAP. -- Holger Krug hkrug@rationalizer.com
Hi, I have a discussion with Teodor over the phone and we agree this is the best for 7.2.*. Thanks Tom for the help. btw, I think it should be noticed somewhere in documentation for developers that pointers to "int" and "long" which are the same on 32-bit machine, are different on the 64-bit machine. Oleg PS. For more than year of our GiST development we got the first report from 64-bit machine and I think it's a good sign. This year we must add concurrency support. We already had discussion with Joseph Hellerstein ( the 'father' of the GiST ) about concurrency support and perhaps we'll go along the paper by Marcel Kornacker. On Fri, 8 Feb 2002, Tom Lane wrote: > Actually, there is a third possibility, which would fix the problem > without requiring any changes in the picksplit functions. You could > do this: > > char *storage; > > storage = palloc(MAXALIGN(VARHDRSZ) + (*len + 1) * sizeof(GISTENTRY)); > entryvec = (bytea *) (storage + MAXALIGN(VARHDRSZ) - VARHDRSZ); > > use entryvec as before, except final pfree is pfree(storage) > > Grotty as heck, but probably the right answer for 7.2.1 to avoid the > initdb issues. > > For 7.3 we could do it the other, cleaner way. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > 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
This patch solve the problem with unaligned access on 64-bit box. Please apply it for 7.2.1. Tested on DEC Alpha. Tom Lane wrote: > Actually, there is a third possibility, which would fix the problem > without requiring any changes in the picksplit functions. You could > do this: > > char *storage; > > storage = palloc(MAXALIGN(VARHDRSZ) + (*len + 1) * sizeof(GISTENTRY)); > entryvec = (bytea *) (storage + MAXALIGN(VARHDRSZ) - VARHDRSZ); > > use entryvec as before, except final pfree is pfree(storage) > > Grotty as heck, but probably the right answer for 7.2.1 to avoid the > initdb issues. > > For 7.3 we could do it the other, cleaner way. > > regards, tom lane > > -- Teodor Sigaev teodor@stack.net
Attachment
We got the report about this patch on 64-bit Solaris: it's work. Teodor Sigaev wrote: > This patch solve the problem with unaligned access on 64-bit box. Please > apply it for 7.2.1. > > Tested on DEC Alpha. > > Tom Lane wrote: > >> Actually, there is a third possibility, which would fix the problem >> without requiring any changes in the picksplit functions. You could >> do this: >> >> char *storage; >> >> storage = palloc(MAXALIGN(VARHDRSZ) + (*len + 1) * >> sizeof(GISTENTRY)); >> entryvec = (bytea *) (storage + MAXALIGN(VARHDRSZ) - VARHDRSZ); >> >> use entryvec as before, except final pfree is pfree(storage) >> >> Grotty as heck, but probably the right answer for 7.2.1 to avoid the >> initdb issues. >> >> For 7.3 we could do it the other, cleaner way. >> >> regards, tom lane >> >> > > > > ------------------------------------------------------------------------ > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Teodor Sigaev teodor@stack.net
Teodor Sigaev <teodor@stack.net> writes:
> This patch solve the problem with unaligned access on 64-bit
> box. Please apply it for 7.2.1.
Patch applied, thanks.
regards, tom lane