Re: updated hstore patch

On Sep 15, 2009, at 8:31 PM, Andrew Gierth wrote:

> Gah. rerolled to fix a missing file. includes the docs too this time.

Yay, thank you Andrew! Here are my review notes.

Testing
=======

Here's what I did to try out the patch, paying special attention to in-
place upgrading:

* I built a simple database with a table with an (old) hstore column
from an unpatched Git checkout.

* I ran the script I developed for testing the previous patch in July,
commenting out the new features, just to test the existing
implementation.

* I applied your patch, rebuilt hstore, installed the DSO, and
restarted PotgreSQL. I did *not* dump the previous database or restore
it, I just just used the existing cluster. The only thing that changed
was the new hstore.

* I ran the hstore `make installcheck` and all tests passed.

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

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

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

* I connected to my simple database and did a select on the table I
created before installing the new hstore, and all the old data showed
up fine in psql.

* I uncommented the new stuff in my test script (attached) and ran it.
Everything worked as I expected.

Notes and minor issues:

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

SELECT 'a=>1, b =>2'::hstore - 'a' = 'b=>2';
ERROR: Unexpected end of string
LINE 1: SELECT 'a=>1, b =>2'::hstore - 'a' = 'b=>2';

But it works if I cast it:

SELECT 'a=>1, b =>2'::hstore - 'a'::text = 'b=>2';

Not sure if there's anything to be done about that. I mentioned this
issue back in July, as well.

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

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

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

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

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

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

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

Comments
========

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

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

* Also, just as an aside, I'm wondering if the approach you've taken
could be used for core data types going forward, so as to work towards
true in-place upgrades in the future?

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

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

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

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

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

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

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

Review Template
===============

Addressing the points in the [wiki](http://wiki.postgresql.org/wiki/Reviewing_a_Patch
):

Submission review
-----------------

* Is the patch in context diff format?

Yes.

* Does it apply cleanly to the current CVS HEAD?

A couple of Fuzz 1s, but otherwise, yes.

* Does it include reasonable tests, necessary doc patches, etc?

Yes.

Usability review
----------------

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Oh yes!

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

Yes, mostly by relying on the precedence of even having this data
type in the first place. Relational purists no doubt hate it already.

* Are there dangers?

With the upgrade isses addressed, I believe that the answer is No.

* Have all the bases been covered?

Yes.

Feature test
------------

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

Not that I could find.

Performance review
------------------

* Does the patch slow down simple tests?

No. `make installcheck` in `contrib/hstore` runs very quickly.

* If it claims to improve performance, does it?

N/A

* Does it slow down other things?

No.

Coding review
-------------

* Does it follow the project coding guidelines?

Yes.

* Are there portability issues?

I'm not qualified to judge. It worked great on my MacBook Pro.

* Will it work on Windows/BSD etc?

I'm not qualified to judge.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

Architecture review
-------------------

* Is everything done in a way that fits together coherently with other
features/modules?

Yes. The use of psql varables to disable transactions and specify a
schema in the install script is particuarly handy. I'd be interested
to see if there's some way they could be generalized for use in
modules in general.

* Are there interdependencies than can cause problems?

No.

Conclusion
==========

As I mentioned last time around, I'd like to do some word-smithing to
the docs, but I'm happy to wait until it's committed and send a new
patch. Otherwise, a few minor documentation issues notwithstanding, I
think that this patch is ready for committer review and, perhaps,
committing. The code looks clean (though mainly over my head) and the
functionality is top-notch.

Best,

David