Re: updated hstore patch

>>>>> "David" == "David E Wheeler" <david(at)kineticode(dot)com> writes:

David> * I ran the following to update the SQL functions in my simple database:

David> psql -d try --set hstore_xact='--' -f hstore.sql

David> The use of `--set hstore_xact='--' was on Andrew's advice
David> via IRC, because otherwise the entire update takes place in
David> a single transaction, and thus would fail since I already
David> had the old hstore installed. By using this psql variable
David> hack to disable the transactions, I was able to install
David> over the old hstore.

I'm more than half inclined to take this bit out again. It made sense in the
context of the pgfoundry version, but it doesn't fit in so well for contrib.
(It's a concept I was testing on the pgfoundry version)

However, I would prefer to keep the ability to do this:

psql --set hstore_schema='myschema' -f hstore.sql dbname

The logic to do it is a bit ugly, but editing the file to set what schema to
use is even uglier...

David> Notes and minor issues:

David> * `hstore - hstore` resolves before `hstore - text`, meaning
David> that this failed:

The '-' operator did not previously exist so this isn't a compatibility issue.
BUT the delete(hstore,text) function did previously exist, however it seems to
still be resolved in preference to delete(hstore,hstore) when the second arg
is a bare text literal:

contrib_regression=# explain verbose select delete(('a' => now()::text),'a');
QUERY PLAN
-----------------------------------------------------------
Result (cost=0.00..0.02 rows=1 width=0)
Output: delete(('a'::text => (now())::text), 'a'::text)
(2 rows)

contrib_regression=# explain verbose select delete(('a' => now()::text),'a=>1'::hstore);
QUERY PLAN
--------------------------------------------------------------------
Result (cost=0.00..0.02 rows=1 width=0)
Output: delete(('a'::text => (now())::text), '"a"=>"1"'::hstore)
(2 rows)

The only open question I can see is what delete(hs,$1) resolves to when $1 is
an unknown-type placeholder; this is probably an incompatibility with the old
version if anyone is relying on that (but I don't see why they would be).

David> * Maybe it's time to kill off `(at)` and `~`, eh? Or could they
David> generate warnings for a release or something? How are
David> operators properly deprecated?

David> * The documentation for `populate_record()` is wrong. It's
David> still just a cut-and-paste of `delete()`

GAH. I fixed some doc issues (stray &s) but forgot that one. I'll fix it.

David> * The NOTE about `populate_record()` says that it takes
David> anyelement instead of record and just throws an error if it's
David> not a record. I thought that C functions could take record
David> arguments. Why do the extra work here?

Because "record" doesn't express the fact that the return type of
populate_record is the SAME record type as its parameter type, whereas
anyelement does.

David> * Your original submission say that the new version offers
David> btree and hash support, but the docs still mention only GIN
David> and GIST.

I'll fix that.

David> * I'd like to see more examples of the new functionality in
David> the documentation. I'd be happy to contribute them once the
David> main patch is committed.

Thanks. I'll do some (especially for populate_record, which really needs
them), but more can be added later.

David> * I think that there should be some exmples in the docs of the
David> use of the backslash with and without
David> standard_conforming_strings and with and without E''. That
David> stuff is confusing enough, it should all be spelled out, IMHO.

ugh. yeah

David> * The use of the `:hstore_xact` variable hack needs to be
David> documented,

(or removed, see above)

David> along with detailed instructions for those upgrading
David> (in-place) from 8.4. You might consider documenting your
David> `:hstore_default_schema` variable as well: if I understand
David> correctly, it's a way to specify a schema on the command-line
David> without having to edit the file, yes? Currently, there are no
David> installation instructions in the documentation.

ya.

David> Comments
David> ========

David> * So the main objection to the original patch from the July
David> CommitFest, that it wasn't backwards compatible, seems to have
David> been addressed, assuming that the structure currently used in
David> HEAD is just like what's in 8.4, so in-place upgrade should
David> work, yes? It apears that you've taken the approach, in
David> hstore_compat.c, of reading both the old and the new
David> formats. Does it also upgrade the old formats as it reads
David> them, or only as they're updated? (I'm assuming the latter.)

Old values are converted to new values in core, but they can't be
changed on-disk unless something actually updates them.

David> * I'm not qualified to talk about the approach taken to
David> maintaining compatibility, but would like to know if it
David> imposes an overhead on the use of the data type, and if so,
David> how much?

The overhead is possibly non-negligible for reading old values, but
old values can be promoted to new ones fairly simply (e.g. using
ALTER TABLE).

David> * Regarding the bug you found in the old hstore format (mentioned
David> [here](http://archives.postgresql.org/pgsql-hackers/2009-07/msg01422.php)
David> ), has that been fixed? Is it a regression that should be
David> back-patched?

That has not been fixed.

David> * Does there need to be documentation with upgrade instructions for
David> hstore-new users (the pgFoundry version)? Or will you handle that via
David> a new pgFoundry release?

There needs to be more documentation before the pgfoundry version gets
a release, yes. I'm still testing some interactions between the three
versions.

David> * In addition to the functions to get all the keys and all the
David> values as arrays, I'd love a function (or, dare I say, a
David> cast?) to get all the rows and keys in an array. Something
David> like this:

David> try=# select 'a => 1, b => 2'::hstore::text[];
David> array
David> -----------
David> {a,1,b,2}

David> I'd find this especially useful in my Perl applications, as
David> then I could easily fetch hstores as arrays and turn them
David> into hashes in my Perl code by simply doing `my %h = @{
David> $row->{hstore} };`. Of course, the inverse would be useful
David> as well:

David> try=# select ARRAY[ 'a', '1', 'b', '2']::hstore;
David> hstore
David> --------------------
David> "a"=>"1", "b"=>"2"

David> A function would work as well, failing community interest
David> in an explicit cast.

I'm not sure I like these as casts but I'm open to opinions. Having them
as functions is certainly doable.

--
Andrew (irc:RhodiumToad)