Re: updated hstore patch

>>>>> "David" == "David E Wheeler" <david@kineticode.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 adviceDavid>    via IRC, because otherwise the entire
updatetakes place inDavid>    a single transaction, and thus would fail since I alreadyDavid>    had the old hstore
installed.By using this psql variableDavid>    hack to disable the transactions, I was able to installDavid>    over
theold 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`, meaningDavid> 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);
QUERYPLAN                             
 
--------------------------------------------------------------------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 `@` and `~`, eh? Or could theyDavid> generate warnings for a release or something?
HowareDavid> operators properly deprecated?
 
David> * The documentation for `populate_record()` is wrong. It'sDavid> 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 takesDavid> anyelement instead of record and just throws an
errorif it'sDavid> not a record. I thought that C functions could take recordDavid> 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 offersDavid> btree and hash support, but the docs still
mentiononly GINDavid> and GIST.
 

I'll fix that.
David> * I'd like to see more examples of the new functionality inDavid> the documentation. I'd be happy to contribute
themonce theDavid> 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 theDavid> use of the backslash with and withoutDavid>
standard_conforming_stringsand with and without E''. ThatDavid> stuff is confusing enough, it should all be spelled
out,IMHO.
 

ugh. yeah
David> * The use of the `:hstore_xact` variable hack needs to beDavid> documented,

(or removed, see above)
David> along with detailed instructions for those upgradingDavid> (in-place) from 8.4. You might consider documenting
yourDavid>`:hstore_default_schema` variable as well: if I understandDavid> correctly, it's a way to specify a schema on
thecommand-lineDavid> without having to edit the file, yes?  Currently, there are noDavid> installation instructions in
thedocumentation.
 

ya.
David> CommentsDavid> ========
David> * So the main objection to the original patch from the JulyDavid> CommitFest, that it wasn't backwards
compatible,seems to haveDavid> been addressed, assuming that the structure currently used inDavid> HEAD is just like
what'sin 8.4, so in-place upgrade shouldDavid> work, yes? It apears that you've taken the approach, inDavid>
hstore_compat.c,of reading both the old and the newDavid> formats. Does it also upgrade the old formats as it
readsDavid>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 toDavid> maintaining compatibility, but would like to know
ifitDavid> 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 (mentionedDavid>
[here](http://archives.postgresql.org/pgsql-hackers/2009-07/msg01422.php)David>), has that been fixed? Is it a
regressionthat should beDavid> back-patched?
 

That has not been fixed.
David> * Does there need to be documentation with upgrade instructions forDavid> hstore-new users (the pgFoundry
version)?Or will you handle that viaDavid> 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 theDavid> values as arrays, I'd love a function (or,
dareI say, aDavid> cast?) to get all the rows and keys in an array. SomethingDavid> like this:
 
David>        try=# select 'a => 1, b => 2'::hstore::text[];David>           arrayDavid>        -----------David>
 {a,1,b,2}
 
David>    I'd find this especially useful in my Perl applications, asDavid>    then I could easily fetch hstores as
arraysand turn themDavid>    into hashes in my Perl code by simply doing `my %h = @{David>    $row->{hstore} };`. Of
course,the inverse would be usefulDavid>    as well:
 
David>        try=# select ARRAY[ 'a', '1', 'b', '2']::hstore;David>               hstoreDavid>
--------------------David>        "a"=>"1", "b"=>"2"
 
David>    A function would work as well, failing community interestDavid>    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)