Обсуждение: Re: updated hstore patch

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

--
Andrew (irc:RhodiumToad)

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 `@` 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

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

On Sat, Sep 19, 2009 at 03:27, Andrew Gierth
<andrew@tao11.riddles.org.uk> 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...

That seems like a pretty good thing to have, but that shouldn't be in
the hstore patch. If we want to do that, we should do it for *all*
contrib modules, so they are consistent.

Which I think would be good, but given previous discussions I'm sure
somebody is going ot have an argument against it...


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

"David E. Wheeler" <david@kineticode.com> writes:
> ... 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.

Given the number of questions in your review, I don't think this is
actually ready to commit.  I'm marking it "waiting on author" instead.
        regards, tom lane

On Sep 19, 2009, at 7:03 PM, Tom Lane wrote:

> Given the number of questions in your review, I don't think this is
> actually ready to commit.  I'm marking it "waiting on author" instead.

Yes, actually, I had second thoughts about that and meant to change it  
myself. Thanks Tom.

David

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> "David E. Wheeler" <david@kineticode.com> writes:>> ... 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.
 
Tom> Given the number of questions in your review, I don't think this isTom> actually ready to commit.  I'm marking it
"waitingon author" instead.
 

I will resubmit (hopefully in a day or two) with the following changes:
- removal of the \set schema variable stuff from the .sql script- documentation fixes as mentioned in my previous
email-additional documentation for some of the newer features- more internal code documentation covering the handling
ofold formats
 

I'd appreciate public feedback on:
- whether conversions to/from a {key,val,key,val,...} array are needed  (and if there's strong opinions in favour of
makingthem casts; in the  absence of strong support for that, I'll stick to functions)
 
- what to do when installing the new version's .sql into an existing db;  the send/recv support and some of the index
supportdoesn't get installed  if the hstore type and opclasses already exist. In the case of an upgrade  (or a
dump/restorefrom an earlier version) it would be nice to make all  the functionality available; but there's no CREATE
ORREPLACE for types  or operator classes.
 

If there are any more potential showstoppers I'd appreciate hearing about
them now rather than later.

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth wrote:
> I'd appreciate public feedback on:
>  - whether conversions to/from a {key,val,key,val,...} array are needed
>    (and if there's strong opinions in favour of making them casts; in the
>    absence of strong support for that, I'll stick to functions)

Strikes me as an independent separate patch.  It seems totally
orthogonal to the features in the patch as submitted, no?

>  - what to do when installing the new version's .sql into an existing db;
>    the send/recv support and some of the index support doesn't get installed
>    if the hstore type and opclasses already exist. In the case of an upgrade
>    (or a dump/restore from an earlier version) it would be nice to make all
>    the functionality available; but there's no CREATE OR REPLACE for types
>    or operator classes.

It seems similar in ways to the PostGIS upgrade issues when their
types and operators change:http://postgis.refractions.net/docs/ch02.html#upgrading
It seems they've settled on a script which processes the dump file
to exclude the parts that would conflict?

If the perfect solution is too complex, I'd also kinda hope this isn't a
show-stopper for this patch, but rather a TODO for the future modules feature.


> If there are any more potential showstoppers I'd appreciate hearing about
> them now rather than later.

Ron Mayer <rm_pg@cheapcomplexdevices.com> writes:
> Andrew Gierth wrote:
>> - what to do when installing the new version's .sql into an existing db;
>> the send/recv support and some of the index support doesn't get installed
>> if the hstore type and opclasses already exist. In the case of an upgrade
>> (or a dump/restore from an earlier version) it would be nice to make all
>> the functionality available; but there's no CREATE OR REPLACE for types
>> or operator classes.

> If the perfect solution is too complex, I'd also kinda hope this isn't a
> show-stopper for this patch, but rather a TODO for the future modules feature.

Yeah, this is a long-standing generic issue, and not really hstore's
problem to fix.
        regards, tom lane

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

On Sep 20, 2009, at 8:43 AM, Tom Lane wrote:

>> If the perfect solution is too complex, I'd also kinda hope this  
>> isn't a
>> show-stopper for this patch, but rather a TODO for the future  
>> modules feature.
>
> Yeah, this is a long-standing generic issue, and not really hstore's
> problem to fix.

So then does there need to be some documentation for how to deal with  
this, for those doing an in-place upgrade from an existing hstore data  
type? Or would that discussion be in Bruce's tool's docs?

Best,

David

On Sep 20, 2009, at 1:03 AM, Andrew Gierth wrote:

> I will resubmit (hopefully in a day or two) with the following  
> changes:
>
> - removal of the \set schema variable stuff from the .sql script
> - documentation fixes as mentioned in my previous email
> - additional documentation for some of the newer features
> - more internal code documentation covering the handling of old  
> formats

+1. And maybe a discussion about upgrading old hstore values?

> I'd appreciate public feedback on:
>
> - whether conversions to/from a {key,val,key,val,...} array are needed
>   (and if there's strong opinions in favour of making them casts; in  
> the
>   absence of strong support for that, I'll stick to functions)

Definitely functions. I'm strongly in favor of an explicit cast, as  
well, but I'm spoiled by Perl, so I may be overly biased. Functions  
will do to start.

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:
> On Sep 20, 2009, at 8:43 AM, Tom Lane wrote:
>> Yeah, this is a long-standing generic issue, and not really hstore's
>> problem to fix.

> So then does there need to be some documentation for how to deal with  
> this, for those doing an in-place upgrade from an existing hstore data  
> type? Or would that discussion be in Bruce's tool's docs?

I'm inclined to correct the existing documentation, which says at the
bottom of http://developer.postgresql.org/pgdocs/postgres/contrib.html
 After a major-version upgrade of PostgreSQL, run the installation script again, even though the module's objects might
havebeen brought forward from the old installation by dump and restore. This ensures that any new functions will be
availableand any needed corrections will be applied.
 

That recipe doesn't actually work for cases like this.  What *would*
work is loading the module *before* restoring from your old dump,
then relying on the CREATEs from the incoming dump to fail.

I believe we have already discussed the necessity for pg_upgrade to
support this type of subterfuge.  A module facility would be a lot
better of course, but we still need something for upgrading existing
databases that don't contain the module structure.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> I believe we have already discussed the necessity for pg_upgrade to
> support this type of subterfuge.  A module facility would be a lot
> better of course, but we still need something for upgrading existing
> databases that don't contain the module structure.

An idea would be to have an external tool to prepare the transition. The
tool would have to be able to build the pg_depend entries for a given
database and a given extension. It could process this way:
- create a new empty database, pg_dump -Ft > empty.dump- install the given extension, pg_dump -Ft > extended.dump-
compareempty and extended dumps catalogs (pg_restore -l)- diffs are from the extension, look them up in given database-
foreach extension's object, try drop cascade it then rollback- parse error messages
 

Now the diff lookup gives a first set of pg_depend entries, which is to
be completed in the DROP CASCASE error parsing step.

Given this we yet have to prepare the database so that pg_dump from
extensions aware major version will be able to dump CREATE and INSTALL
extension commands rather than the extension.sql install file. This can
be done by installing the newer extension on the target database and
point the tool to this, in order to drain the needed catalog entries.

It'll be slow and will take AccessExclusive locks, but you can do it on
a staging server. The output should be a SQL script filling pg_extension
and pg_depend on the existing database.

So user steps are:- pg_addextension <olddb> <newdb> <extname> <install.sql> [...] > exts.sql- psql -f exts.sql <olddb>

From there pg_dump from new version is happy.

Regards,
-- 
dim

PS: once more, devil is the details, and the extension code is to be
written. Hope doing so for 11/15 commitfest, over free time.

>>>>> "David" == David E Wheeler <david@kineticode.com> writes:
>> The only open question I can see is what delete(hs,$1) resolves to>> when $1 is an unknown-type placeholder; this is
probablyan>> incompatibility with the old version if anyone is relying on that>> (but I don't see why they would be).
 
David> Given your examples, I think it probably should resolve toDavid> text as it does, as deleting a single key is
likelyto be aDavid> common case. It should otherwise be cast.
 

I think you're missing the point here; I can't control what it resolves
to, since that's the job of the function overload resolution code.

But I checked, and delete(hstore,$1) still resolves to
delete(hstore,text) when the type of $1 is not specified, so there's
no compatibility issue there that I can see. (I'm not sure I
understand _why_ it resolves to that rather than being ambiguous...)
>> 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> So then it's negligible for new values?

Yes. (One bit test, done inline)

-- 
Andrew.

On Sep 20, 2009, at 3:14 PM, Andrew Gierth wrote:

> I think you're missing the point here; I can't control what it  
> resolves
> to, since that's the job of the function overload resolution code.

Yeah, but I think that the existing behavior is probably the best.

> But I checked, and delete(hstore,$1) still resolves to
> delete(hstore,text) when the type of $1 is not specified, so there's
> no compatibility issue there that I can see. (I'm not sure I
> understand _why_ it resolves to that rather than being ambiguous...)

Right, but it does seem like it might be the best choice for now. I'd  
add a regression test to make sure it stays that way.

> David> So then it's negligible for new values?
>
> Yes. (One bit test, done inline)

Excellent, thanks.

David

On Sep 20, 2009, at 12:15 PM, Tom Lane wrote:

> That recipe doesn't actually work for cases like this.  What *would*
> work is loading the module *before* restoring from your old dump,
> then relying on the CREATEs from the incoming dump to fail.

Jesus this is hacky, either way. :-(

> I believe we have already discussed the necessity for pg_upgrade to
> support this type of subterfuge.  A module facility would be a lot
> better of course, but we still need something for upgrading existing
> databases that don't contain the module structure.

Yeah, it's past time for a real module facility.

Best,

David

>>>>> "David" == "David E Wheeler" <david@kineticode.com> writes:
>> But I checked, and delete(hstore,$1) still resolves to>> delete(hstore,text) when the type of $1 is not specified,
sothere's>> no compatibility issue there that I can see. (I'm not sure I>> understand _why_ it resolves to that rather
thanbeing ambiguous...)
 
David> Right, but it does seem like it might be the best choice forDavid> now. I'd add a regression test to make sure
itstays that way.
 

I don't think there's any way to do that from the regression tests.

-- 
Andrew (irc:RhodiumToad)

On Sep 21, 2009, at 4:57 PM, Andrew Gierth wrote:

> I don't think there's any way to do that from the regression tests.

The output that you demonstrated a few messages back should do nicely  
for delete(), at least:

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);                            QUERY PLAN
--------------------------------------------------------------------
Result  (cost=0.00..0.02 rows=1 width=0)  Output: delete(('a'::text => (now())::text), '"a"=>"1"'::hstore)
(2 rows)

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:
> On Sep 21, 2009, at 4:57 PM, Andrew Gierth wrote:
>> I don't think there's any way to do that from the regression tests.

> The output that you demonstrated a few messages back should do nicely  
> for delete(), at least:

Anything involving 'explain verbose' output is not getting into the
regression tests.  It's not stable enough.

In practice, I don't see why simply testing the result of the function
isn't enough for this...
        regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> On Sep 21, 2009, at 4:57 PM, Andrew Gierth wrote:>>> I don't think there's any way to do that from the regression
tests.
>> The output that you demonstrated a few messages back should do nicely  >> for delete(), at least:
Tom> Anything involving 'explain verbose' output is not getting into theTom> regression tests.  It's not stable
enough.
Tom> In practice, I don't see why simply testing the result of theTom> function isn't enough for this...

Is delete('...'::hstore,'foo') guaranteed to resolve to the same
function as delete('...'::hstore,$1) where $1 has no type specified?

If so, then the regression tests already cover this.

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Is delete('...'::hstore,'foo') guaranteed to resolve to the same
> function as delete('...'::hstore,$1) where $1 has no type specified?

Yup.  They're both UNKNOWN.
        regards, tom lane

Tom Lane wrote:
> "David E. Wheeler" <david@kineticode.com> writes:
> > On Sep 20, 2009, at 8:43 AM, Tom Lane wrote:
> >> Yeah, this is a long-standing generic issue, and not really hstore's
> >> problem to fix.
> 
> > So then does there need to be some documentation for how to deal with  
> > this, for those doing an in-place upgrade from an existing hstore data  
> > type? Or would that discussion be in Bruce's tool's docs?
> 
> I'm inclined to correct the existing documentation, which says at the
> bottom of http://developer.postgresql.org/pgdocs/postgres/contrib.html
> 
>   After a major-version upgrade of PostgreSQL, run the installation script
>   again, even though the module's objects might have been brought forward
>   from the old installation by dump and restore. This ensures that any new
>   functions will be available and any needed corrections will be applied.
> 
> That recipe doesn't actually work for cases like this.  What *would*
> work is loading the module *before* restoring from your old dump,
> then relying on the CREATEs from the incoming dump to fail.
> 
> I believe we have already discussed the necessity for pg_upgrade to
> support this type of subterfuge.  A module facility would be a lot
> better of course, but we still need something for upgrading existing
> databases that don't contain the module structure.

Would someone please explain what needs to be done here?  This is the
original email but I can't figure out what to do about it:
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01368.php

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.comPG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard
drive,Christ can be your backup. +