Re: [HACKERS] updated hstore patch
Tom Lane wrote: David E. Wheeler da...@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 br...@momjian.ushttp://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. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
David == David E Wheeler da...@kineticode.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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
David E. Wheeler da...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
Tom == Tom Lane t...@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 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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
Andrew Gierth and...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
Tom == Tom Lane t...@sss.pgh.pa.us writes:
David E. Wheeler da...@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 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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
Ron Mayer rm...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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?
Davidtry=# select 'a = 1, b = 2'::hstore::text[];
David array
David---
David {a,1,b,2}
DavidI'd find this especially useful in my Perl applications, as
Davidthen I could easily fetch hstores as arrays and turn them
Davidinto hashes in my Perl code by simply doing `my %h = @{
David$row-{hstore} };`. Of course, the inverse would be useful
Davidas well:
Davidtry=# select ARRAY[ 'a', '1', 'b', '2']::hstore;
David hstore
David
David a=1, b=2
DavidA function would work as well, failing community interest
Davidin 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
David E. Wheeler da...@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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Upgrading towards managed extensions (was Re: [HACKERS] updated hstore patch)
Tom Lane t...@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 - 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
David == David E Wheeler da...@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 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
On Sat, Sep 19, 2009 at 03:27, Andrew Gierth and...@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 Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
David E. Wheeler da...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated hstore patch
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
Re: [HACKERS] updated hstore patch
David == David E Wheeler da...@kineticode.com writes:
David * I ran the following to update the SQL functions in my simple database:
Davidpsql -d try --set hstore_xact='--' -f hstore.sql
DavidThe use of `--set hstore_xact='--' was on Andrew's advice
Davidvia IRC, because otherwise the entire update takes place in
Davida single transaction, and thus would fail since I already
Davidhad the old hstore installed. By using this psql variable
Davidhack to disable the transactions, I was able to install
Davidover 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 `...@` 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
Re: [HACKERS] updated hstore patch
Gah. rerolled to fix a missing file. includes the docs too this time. -- Andrew (irc:RhodiumToad) hstore-20090915.patch.gz Description: hstore patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
