Re: revised hstore patch

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> Revision to previous hstore patch to fix (and add tests for) some edge
> case bugs with nulls or empty arrays.

I took a quick look at this, and have a couple of beefs associated with
upgrade risks.

1. The patch arbitrarily changes the C-code names of several existing
SQL functions. DO NOT DO THIS. If somebody dumps an 8.4 database
containing hstore, and loads it into 8.5, the result would be crashes
and perhaps even exploitable security holes. The C name is part of the
function's ABI and you can't just change it. It's okay if, after such
a dump and reload scenario, there is functionality that's inaccessible
because the necessary SQL function definitions are missing. It's not
so okay if there are security holes.

2. The patch changes the on-disk representation of hstore. That is
clearly necessary to achieve the goal of allowing keys/values longer
than 64K, but it breaks on-disk compatibility from 8.4 to 8.5. I'm not
sure what our threshold is for allowing compatibility breaks, but I
think it's higher than this. The demand for longer values inside an
hstore has not been very great.

Perhaps an appropriate thing to do is separate out the representation
change from the other new features, and apply just the latter for now.
Or maybe we should think about having two versions of hstore. This
is all tied up in the problem of having a decent module infrastructure
(which I hope somebody is working on for 8.5). I don't know where
we're going to end up for 8.5, but I'm disinclined to let a fairly
minor contrib feature improvement break upgrade-compatibility before
we've even really started the cycle.

regards, tom lane