Re: updated hstore patch - Mailing list pgsql-hackers

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