Review: revised hstore patch

On Jul 16, 2009, at 7:17 AM, Andrew Gierth wrote:

> Revision to previous hstore patch to fix (and add tests for) some edge
> case bugs with nulls or empty arrays.

This looks great, Andrew, thanks. Here's what I did to try it out:

* I built a simple database with a table with an (old) hstore column.

* I put in some data and wrote a bit of simple SQL to test the
existing implementation, functions, etc., as documented.

* I dumped the data.

* I applied your patch, rebuilt hstore, installed the DSO, and
restarted PotgreSQL.

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

* I dropped the test database, created a new one, and installed hstore
into it.

* I restored the dump and ran my little regression. All the behavior
was the same. The only difference was that `hstore = hstre` started to
work instead of dying -- yay!

* I then did some experimentation to make sure that all of the new
functions and operators worked as documented. They did. I attach my
fiddling for your amusement.

Notes and minor issues:

* This line in the docs:

       <entry><literal>'a=>1,b=>2'::hstore ?& ARRAY['a','b']</
literal></entry>

   Needs "?&" changed to "?&

* `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.

* There are a few operators that take text or a text array as the left
operand, such as `-` and `->`, but not with `?`. This is because the `?
` operator, which returns true if an hstore has a particular key, can
have two meanings when the left operand is an array: either it has all
the keys or it has some of the keys in the array. This patch avoids
this issue by making the former `?&` and the latter `?|`. I appreciate
the distinction, but wanted to point out that it is at the price of
inconsistency vis-a-vis some other operators (that, it must be said,
don't have the three-branch logic to deal with). I think it's a good
call, though.

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

* The conversion between records and hstores and using hstores to
modify particular values in records is hot.

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

* The NOTE about `populoate_record()` says that it takes anyelement
instead of record and just throws an error if it's not a record. I'm
sure there's a good reason for that, but maybe there's a better way?

* 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 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