Re: updated hstore patch

On Sep 18, 2009, at 6:27 PM, Andrew Gierth wrote:

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

Yes, this should perhaps be generalized into a separate patch. I
completely agree that it'd be useful and desirable.

> 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).

Given your examples, I think it probably should resolve to text as it
does, as deleting a single key is likely to be a common case. It
should otherwise be cast.

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

The lack of expressivity in records is beginning to annoy me.

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

Agreed. As I said, once this is committed I'll likely go over the docs
and submit a patch myself.

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

Right, of course, thanks.

> 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).

So then it's negligible for new values?

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

Should it be? I realize that it's a separate issue from this patch,
and maybe it's too edge-case to bother with, given that no one has
complained and it obviously won't exist once your patch is applied.
Right?

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

I think a function would be great here. A cast is something we can
decide to add later, or one can add manually using the function.

Best,

David