small hstore bugfixes for 9.0 (w/patch)

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.