Thread: small hstore bugfixes for 9.0 (w/patch)
Two small fixes for hstore-new. The hstore_compat one is arguable as to what is the best approach; the assert that was there was just wrong, but I have been unable after considerable searching to find any architectures that would fail the check. The version in this patch will just treat any old-format non-empty hstore as being invalid on a platform where the upgrade to the new format would fail. (And this version _is_ tested on at least i386 and amd64, where upgrading works.) The gist one is just that the old code was abusing DatumGetHStoreP by applying it to something that wasn't an hstore. This didn't matter before the format upgrade code was put in, and it didn't show up in tests because you need to index a very large number of hstores before any problem shows up. -- Andrew (irc:RhodiumToad)
Attachment
Andrew Gierth <andrew@tao11.riddles.org.uk> writes: > Two small fixes for hstore-new. > The hstore_compat one is arguable as to what is the best approach; the > assert that was there was just wrong, but I have been unable after > considerable searching to find any architectures that would fail the > check. [ scratches head... ] It looks like that ought to be an immediate core-dump for old data, given an assert-enabled build. Are you saying it isn't? How? regards, tom lane
Andrew Gierth <andrew@tao11.riddles.org.uk> writes: > The gist one is just that the old code was abusing DatumGetHStoreP by > applying it to something that wasn't an hstore. This didn't matter > before the format upgrade code was put in, and it didn't show up in > tests because you need to index a very large number of hstores before > any problem shows up. Actually, since ghstore is not marked toastable (and hardly needs to be, since its max length is 24 bytes), that function seems completely useless. Why isn't it just PG_RETURN_POINTER(PG_GETARG_POINTER(0)); (compare gbt_decompress in btree_gist, for instance). regards, tom lane
I wrote: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> The hstore_compat one is arguable as to what is the best approach; the >> assert that was there was just wrong, but I have been unable after >> considerable searching to find any architectures that would fail the >> check. > [ scratches head... ] It looks like that ought to be an immediate > core-dump for old data, given an assert-enabled build. Are you > saying it isn't? How? I tried this, and indeed an assert-enabled hstore dumps core instantly on a pg_upgraded table. So that upgrade path obviously hasn't been tested very well. But I don't see why we don't just fix the Assert. regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> Two small fixes for hstore-new. >> The hstore_compat one is arguable as to what is the best approach; the >> assert that was there was just wrong, but I have been unable after >> considerable searching to find any architectures that would fail the >> check. Tom> [ scratches head... ] It looks like that ought to be an Tom> immediate core-dump for old data, given an assert-enabled build. Tom> Are you saying it isn't? How? The assert was just wrong, as I said. (Obviously it somehow escaped testing; it's possible that I did my original tests on a non-asserts build by mistake.) What I meant to say is that I couldn't find any architectures that would fail what the check _should have been_. The reason for dropping the assert and doing the check in actual code is because if any platform does exist where the check fails, you'd just get corrupt results in a non-asserts build. I figured it was better to produce an actual error instead. -- Andrew.
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> The gist one is just that the old code was abusing DatumGetHStoreP >> by applying it to something that wasn't an hstore. This didn't >> matter before the format upgrade code was put in, and it didn't >> show up in tests because you need to index a very large number of >> hstores before any problem shows up. Tom> Actually, since ghstore is not marked toastable (and hardly Tom> needs to be, since its max length is 24 bytes), that function Tom> seems completely useless. Why isn't it just Tom> PG_RETURN_POINTER(PG_GETARG_POINTER(0)); Tom> (compare gbt_decompress in btree_gist, for instance). I don't know. The function was like that before I got involved with it; I can only assume it was cargo-culted in from some data type in which the gist keys were toastable. It looks a whole lot like several of Oleg and Teodor's other GiST modules (e.g. ltree, pg_trgm - I suspect that the pg_trgm one is just as useless, though I haven't read enough of that code to be sure.) -- Andrew.