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

>>>>> "David" == "David E Wheeler" <david(at)kineticode(dot)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 advice
David> via IRC, because otherwise the entire update takes place in
David> a single transaction, and thus would fail since I already
David> had the old hstore installed. By using this psql variable
David> hack to disable the transactions, I was able to install
David> over the old 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`, meaning
David> 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);
QUERY PLAN
--------------------------------------------------------------------
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 `(at)` and `~`, eh? Or could they
David> generate warnings for a release or something? How are
David> operators properly deprecated?

David> * The documentation for `populate_record()` is wrong. It's
David> 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 takes
David> anyelement instead of record and just throws an error if it's
David> not a record. I thought that C functions could take record
David> 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 offers
David> btree and hash support, but the docs still mention only GIN
David> and GIST.

I'll fix that.

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.

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

ugh. yeah

David> * The use of the `:hstore_xact` variable hack needs to be
David> documented,

(or removed, see above)

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

ya.

David> Comments
David> ========

David> * So the main objection to the original patch from the July
David> CommitFest, that it wasn't backwards compatible, seems to have
David> been addressed, assuming that the structure currently used in
David> HEAD is just like what's in 8.4, so in-place upgrade should
David> work, yes? It apears that you've taken the approach, in
David> hstore_compat.c, of reading both the old and the new
David> formats. Does it also upgrade the old formats as it reads
David> 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 to
David> maintaining compatibility, but would like to know if it
David> 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 (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.

David> * Does there need to be documentation with upgrade instructions for
David> hstore-new users (the pgFoundry version)? Or will you handle that via
David> 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 the
David> values as arrays, I'd love a function (or, dare I say, a
David> cast?) to get all the rows and keys in an array. Something
David> like this:

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.

--
Andrew (irc:RhodiumToad)

On Sat, Sep 19, 2009 at 03:27, Andrew Gierth
<andrew(at)tao11(dot)riddles(dot)org(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

"David E. Wheeler" <david(at)kineticode(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:

> "David E. Wheeler" <david(at)kineticode(dot)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 is
Tom> actually ready to commit. I'm marking it "waiting on 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 of old 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 making them 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 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 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(at)cheapcomplexdevices(dot)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(at)kineticode(dot)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.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
- compare empty and extended dumps catalogs (pg_restore -l)
- diffs are from the extension, look them up in given database
- for each 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(at)kineticode(dot)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 probably an
>> 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 to
David> text as it does, as deleting a single key is likely to be a
David> 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(at)kineticode(dot)com> writes:

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

David> Right, but it does seem like it might be the best choice for
David> now. I'd add a regression test to make sure it stays 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(at)kineticode(dot)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(at)sss(dot)pgh(dot)pa(dot)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 the
Tom> regression tests. It's not stable enough.

Tom> In practice, I don't see why simply testing the result of the
Tom> 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(at)tao11(dot)riddles(dot)org(dot)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(at)kineticode(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +