Re: revised hstore patch

Revision to previous hstore patch to fix (and add tests for) some edge
case bugs with nulls or empty arrays.

--
Andrew (irc:RhodiumToad)

On Jul 16, 2009, at 7:17 AM, Andrew Gierth wrote:

> Revision to previous hstore patch to fix (and add tests for) some edge
> case bugs with nulls or empty arrays.

This looks great, Andrew, thanks. Here's what I did to try it out:

* I built a simple database with a table with an (old) hstore column.

* I put in some data and wrote a bit of simple SQL to test the
existing implementation, functions, etc., as documented.

* I dumped the data.

* I applied your patch, rebuilt hstore, installed the DSO, and
restarted PotgreSQL.

* I ran the hstore `make installcheck` and all tests passed.

* I dropped the test database, created a new one, and installed hstore
into it.

* I restored the dump and ran my little regression. All the behavior
was the same. The only difference was that `hstore = hstre` started to
work instead of dying -- yay!

* I then did some experimentation to make sure that all of the new
functions and operators worked as documented. They did. I attach my
fiddling for your amusement.

Notes and minor issues:

* This line in the docs:

<entry><literal>'a=&gt;1,b=&gt;2'::hstore ?& ARRAY['a','b']</
literal></entry>

Needs "?&" changed to "?&amp;

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

* There are a few operators that take text or a text array as the left
operand, such as `-` and `->`, but not with `?`. This is because the `?
` operator, which returns true if an hstore has a particular key, can
have two meanings when the left operand is an array: either it has all
the keys or it has some of the keys in the array. This patch avoids
this issue by making the former `?&` and the latter `?|`. I appreciate
the distinction, but wanted to point out that it is at the price of
inconsistency vis-a-vis some other operators (that, it must be said,
don't have the three-branch logic to deal with). I think it's a good
call, though.

* 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 conversion between records and hstores and using hstores to
modify particular values in records is hot.

* The documentation for `populate_record()` is wrong. It's just a cut-
and-paste of `delete()`

* The NOTE about `populoate_record()` says that it takes anyelement
instead of record and just throws an error if it's not a record. I'm
sure there's a good reason for that, but maybe there's a better way?

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

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> Revision to previous hstore patch to fix (and add tests for) some edge
> case bugs with nulls or empty arrays.

I took a quick look at this, and have a couple of beefs associated with
upgrade risks.

1. The patch arbitrarily changes the C-code names of several existing
SQL functions. DO NOT DO THIS. If somebody dumps an 8.4 database
containing hstore, and loads it into 8.5, the result would be crashes
and perhaps even exploitable security holes. The C name is part of the
function's ABI and you can't just change it. It's okay if, after such
a dump and reload scenario, there is functionality that's inaccessible
because the necessary SQL function definitions are missing. It's not
so okay if there are security holes.

2. The patch changes the on-disk representation of hstore. That is
clearly necessary to achieve the goal of allowing keys/values longer
than 64K, but it breaks on-disk compatibility from 8.4 to 8.5. I'm not
sure what our threshold is for allowing compatibility breaks, but I
think it's higher than this. The demand for longer values inside an
hstore has not been very great.

Perhaps an appropriate thing to do is separate out the representation
change from the other new features, and apply just the latter for now.
Or maybe we should think about having two versions of hstore. This
is all tied up in the problem of having a decent module infrastructure
(which I hope somebody is working on for 8.5). I don't know where
we're going to end up for 8.5, but I'm disinclined to let a fairly
minor contrib feature improvement break upgrade-compatibility before
we've even really started the cycle.

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

Tom> Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
>> Revision to previous hstore patch to fix (and add tests for) some edge
>> case bugs with nulls or empty arrays.

Tom> I took a quick look at this, and have a couple of beefs
Tom> associated with upgrade risks.

Tom> 1. The patch arbitrarily changes the C-code names of several
Tom> existing SQL functions.

(a) As written, it provides all of the old names too.

(b) many of the old names are significant collision risks.

(This was all discussed previously. I specifically said that
compatibility was being maintained on this point; you obviously missed
that.)

Tom> 2. The patch changes the on-disk representation of hstore. That
Tom> is clearly necessary to achieve the goal of allowing keys/values
Tom> longer than 64K, but it breaks on-disk compatibility from 8.4 to
Tom> 8.5. I'm not sure what our threshold is for allowing
Tom> compatibility breaks, but I think it's higher than this. The
Tom> demand for longer values inside an hstore has not been very
Tom> great.

The intention is that hstore(record) should work for all practically
useful record sizes. While it's possible for records to be much
larger than 1GB, in practice you're going to run into issues long
before then. Conversely, text fields over 64k are much more common.

The code already has users who are using it for audit-trail stuff
(easily computing the changes between old and new records and storing
only the differences). Perhaps one of the existing users could express
an opinion on this point.

Certainly when developing this I had _SIGNIFICANT_ encouragement, some
of it from YOU, for increasing the limit. (see for example
http://archives.postgresql.org/pgsql-hackers/2009-03/msg00577.php or
http://archives.postgresql.org/pgsql-hackers/2009-03/msg00607.php in
which alternative limits are discussed; I only noticed later that it
was possible to increase the limit to 1GB for both keys and values
without using extra space.)

Tom> Perhaps an appropriate thing to do is separate out the
Tom> representation change from the other new features, and apply
Tom> just the latter for now. Or maybe we should think about having
Tom> two versions of hstore.

Both of those options suck (and I don't believe either would suit users
of the code).

I'm prepared to give slightly more consideration to option #3: make
the new code read the old format as well as the new one. I believe
(though I have not yet tested) that it is possible to reliably
distinguish the two with relatively low overhead, though the overhead
would be nonzero, and do an in-core format conversion (which would
result in writing out the new format if anything changed).

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> (b) many of the old names are significant collision risks.

What collision risks? PG does not allow loadable libraries to export
their symbols, so I don't see the problem. I recommend just keeping
those things named as they were.

> Certainly when developing this I had _SIGNIFICANT_ encouragement, some
> of it from YOU, for increasing the limit.

Yes, but that was before the interest in in-place upgrade went up.
This patch is the first place where we have to decide whether we meant
it when we said we were going to pay more attention to that.

> I'm prepared to give slightly more consideration to option #3: make
> the new code read the old format as well as the new one.

If you think you can make that work, it would solve the problem.

regards, tom lane

On Wed, 2009-07-22 at 01:06 +0100, Andrew Gierth wrote:
> I'm prepared to give slightly more consideration to option #3: make
> the new code read the old format as well as the new one. I believe
> (though I have not yet tested) that it is possible to reliably
> distinguish the two with relatively low overhead, though the overhead
> would be nonzero, and do an in-core format conversion (which would
> result in writing out the new format if anything changed).

It might be good to have a way to ensure that all values have been
upgraded to the new format. Otherwise you have to permanently maintain
the old format even across multiple upgrades. Maybe that's not a big
deal, but I thought I'd bring it up.

Regards,
Jeff Davis

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> (b) many of the old names are significant collision risks.

Tom> What collision risks? PG does not allow loadable libraries to
Tom> export their symbols, so I don't see the problem. I recommend
Tom> just keeping those things named as they were.

You've never tested this, I can tell.

I specifically checked this point, back when working on the original
proposal (and when debugging the uuid code on freebsd, where uuid-ossp
crashes due to a symbol collision). If a loaded module compiled from
multiple source files defines some symbol, and another similar loaded
module tries to define the same symbol, then whichever one gets loaded
second will end up referring to the one from the first, obviously
causing hilarity to ensue.

I have a test case that demonstrates this and everything:

% bin/psql -c 'select foo()' postgres
NOTICE: mod1a foo() called
NOTICE: mod1b bar() called
foo
-----

(1 row)

% bin/psql -c 'select baz()' postgres
NOTICE: mod2a baz() called
NOTICE: mod2b bar() called
baz
-----

(1 row)

% bin/psql -c 'select baz(),foo()' postgres
NOTICE: mod2a baz() called
NOTICE: mod2b bar() called
NOTICE: mod1a foo() called
NOTICE: mod2b bar() called
baz | foo
-----+-----
|
(1 row)

% bin/psql -c 'select foo(),baz()' postgres
NOTICE: mod1a foo() called
NOTICE: mod1b bar() called
NOTICE: mod2a baz() called
NOTICE: mod1b bar() called
foo | baz
-----+-----
|
(1 row)

Notice that in the third case, foo() called the bar() function in mod2b,
not the one in mod1b which it called in the first case. All modules are
compiled with pgxs and no special options.

>> Certainly when developing this I had _SIGNIFICANT_ encouragement, some
>> of it from YOU, for increasing the limit.

Tom> Yes, but that was before the interest in in-place upgrade went up.
Tom> This patch is the first place where we have to decide whether we meant
Tom> it when we said we were going to pay more attention to that.

>> I'm prepared to give slightly more consideration to option #3: make
>> the new code read the old format as well as the new one.

Tom> If you think you can make that work, it would solve the problem.

OK. Should I produce an additional patch, or re-do the original one?

--
Andrew.

>>>>> "Jeff" == Jeff Davis <pgsql(at)j-davis(dot)com> writes:

>> I'm prepared to give slightly more consideration to option #3:
>> make the new code read the old format as well as the new one. I
>> believe (though I have not yet tested) that it is possible to
>> reliably distinguish the two with relatively low overhead, though
>> the overhead would be nonzero, and do an in-core format conversion
>> (which would result in writing out the new format if anything
>> changed).

Jeff> It might be good to have a way to ensure that all values have
Jeff> been upgraded to the new format. Otherwise you have to
Jeff> permanently maintain the old format even across multiple
Jeff> upgrades. Maybe that's not a big deal, but I thought I'd bring
Jeff> it up.

Running ALTER TABLE foo ALTER COLUMN bar TYPE hstore USING bar || '';
on all of your hstore columns would suffice to ensure that, I believe.
(Subject to testing once I actually have code for it, of course.)

--
Andrew (irc:RhodiumToad)

* Andrew Gierth (andrew(at)tao11(dot)riddles(dot)org(dot)uk) wrote:
> Running ALTER TABLE foo ALTER COLUMN bar TYPE hstore USING bar || '';
> on all of your hstore columns would suffice to ensure that, I believe.
> (Subject to testing once I actually have code for it, of course.)

This could/would be included in pg_migrator then, I would think..

Stephen

On Tue, Jul 21, 2009 at 7:25 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Or maybe we should think about having two versions of hstore.  This
> is all tied up in the problem of having a decent module infrastructure
> (which I hope somebody is working on for 8.5).

A decent module infrastructure is probably not going to fix this
problem unless it links with -ldwiw. There are really only two
options here:

- Keep the old version around for compatibility and add a new version
that isn't compatible, plus provide a migration path from the old
version to the new version.

- Make the new version read the format written by the old version.

(I am also not aware that anyone is working on the module
infrastructure problem, though of course that doesn't mean that no-one
is; but the point is that's neither here nor there as respects the
present problem. The module infrastructure is just a management layer
around the same underlying issues.)

...Robert

Andrew Gierth wrote:
> The code already has users who are using it for audit-trail stuff
> (easily computing the changes between old and new records and storing
> only the differences). Perhaps one of the existing users could express
> an opinion on this point.
>
>
>
I use it for exactly that purpose (and it works extremely well). I'm not
sure we have any values > 64k, though, and certainly our keys are tiny -
they are all column names. OTOH, that could well be an annoying
limitation, and would be easily breached if the changed field were some
binary object like an image or a PDF.

I rather like your idea of doing a convert-on-write, if you can reliably
detect that the data is in the old or new format.

cheers

andrew

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> I'm prepared to give slightly more consideration to option #3:
>> make the new code read the old format as well as the new one.

Tom> If you think you can make that work, it would solve the problem.

I was hoping to do it without changing the new format, but that turns
out not to be achievable, thanks to the fact (which I just discovered)
that the old hstore can leave unlimited amounts of wasted space at the
end of the object (does this count as a bug?).

It's doable via a small change to the new format of course. This would
require some care in handling the (few) users of pgfoundry hstore-new,
but all that means is that users of the pre-release hstore-new would
have to ensure at least one update of stored data before doing a binary
migrate to 8.5, which I think isn't too much of a burden.

The other option would be to fix the wasted-space bug in the existing
hstore, and document that stored data must be updated at least once
subsequent to that fix before doing a binary migrate to 8.5.

(The pathological case is _very_ rare, requiring an 0-length key with
exactly 32kb of data, followed by a specific series of key lengths
with values of length 1, and with more than 28kbytes of wasted space
at the end of the value, and only on little-endian machines. The only
way to get that much wasted space is to do several thousand iterations
of UPDATE ... SET hs = hs || ''; each of which adds 8 bytes of wasted
space to the end of the value. But even so, somebody, somewhere,
probably has a value that matches...)

--
Andrew (irc:RhodiumToad)

On Jul 22, 2009, at 8:55 AM, Andrew Gierth wrote:

> The other option would be to fix the wasted-space bug in the existing
> hstore, and document that stored data must be updated at least once
> subsequent to that fix before doing a binary migrate to 8.5.

Given this…

> (The pathological case is _very_ rare, requiring an 0-length key with
> exactly 32kb of data, followed by a specific series of key lengths
> with values of length 1, and with more than 28kbytes of wasted space
> at the end of the value, and only on little-endian machines. The only
> way to get that much wasted space is to do several thousand iterations
> of UPDATE ... SET hs = hs || ''; each of which adds 8 bytes of wasted
> space to the end of the value. But even so, somebody, somewhere,
> probably has a value that matches...)

Could it be that only folks with such a manifestation of the bug would
need to do a binary upgrade? If so, I certainly think it'd be worth it
to fix the bug.

Best,

David

Hi,

Le 22 juil. 09 à 02:56, Robert Haas a écrit :
> On Tue, Jul 21, 2009 at 7:25 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Or maybe we should think about having two versions of hstore. This
>> is all tied up in the problem of having a decent module
>> infrastructure
>> (which I hope somebody is working on for 8.5).

I indeed still intend to provide some patch in the 8.5 cycle. While
the user design issue didn't receive any push back, some big items
remain to be solved. So here's my current TODO about it:

- get some more familiar and involved in backend code by being one
of the RRR
- consider the proposed syntax ok for a first stab at it
- make the pg_catalog.pg_extension entry and the associated commands
with version as text, one thing at a time please
- bootstrap core components in pg_extension for dependancy on them
(plpgsql, ...)
- implement a backend function pg_execute_commands_from_file('path/
to/file.sql');
reserved to superuser, file in usual accepted places
- implement INSTALL EXTENSION with the previous function
- adding a static backend local variable installing_extension (oid)
- modifying each SQL object create statement to add entries in
pg_depend
- add an specific type for handling version numbers and operators
comparing them

Here are from memory the problems we don't have a solution for yet:
- how to give user the ability to install the extension's objects in
another schema than the pg_extension default one
- how to provide extension author a way to have major PG version
dependant code without having to implement and maintain a specific
function in their install.sql file

Please go comment on this other thread if you think the syntax is
awful or for helping me through the big tickets:
http://archives.postgresql.org/pgsql-hackers/2009-06/msg01281.php

> A decent module infrastructure is probably not going to fix this
> problem unless it links with -ldwiw. There are really only two
> options here:

I beg to defer. The way for a decent *extension* facility to handle
the case is by providing an upgrade function which accepts too
arguments: old and new version of the module. Then the module author
is able to run custom code from within the module upgrade transaction,
where migrating on disk data representation is entirely possible.
pg_depend would have to allow for easy finding of a given datatype
column I guess.

> (I am also not aware that anyone is working on the module
> infrastructure problem, though of course that doesn't mean that no-one
> is; but the point is that's neither here nor there as respects the
> present problem. The module infrastructure is just a management layer
> around the same underlying issues.)

Of course if anyone wants to join, I'd appreciate. Some have offered
help and I've been failing to provide them with my todo list... but
getting a first patch for next commit fest is a goal.

Regards,
--
dim

On Wed, Jul 22, 2009 at 1:40 PM, Dimitri Fontaine<dfontaine(at)hi-media(dot)com> wrote:
> Hi,
>
> Le 22 juil. 09 à 02:56, Robert Haas a écrit :
>>
>> On Tue, Jul 21, 2009 at 7:25 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>> Or maybe we should think about having two versions of hstore.  This
>>> is all tied up in the problem of having a decent module infrastructure
>>> (which I hope somebody is working on for 8.5).
>
> I indeed still intend to provide some patch in the 8.5 cycle. While the user
> design issue didn't receive any push back, some big items remain to be
> solved. So here's my current TODO about it:
>
>  - get some more familiar and involved in backend code by being one of the
> RRR
>  - consider the proposed syntax ok for a first stab at it
>  - make the pg_catalog.pg_extension entry and the associated commands
>     with version as text, one thing at a time please
>  - bootstrap core components in pg_extension for dependancy on them
> (plpgsql, ...)
>  - implement a backend function
> pg_execute_commands_from_file('path/to/file.sql');
>     reserved to superuser, file in usual accepted places
>  - implement INSTALL EXTENSION with the previous function
>    - adding a static backend local variable installing_extension (oid)
>    - modifying each SQL object create statement to add entries in pg_depend
>  - add an specific type for handling version numbers and operators comparing
> them
>
> Here are from memory the problems we don't have a solution for yet:
>  - how to give user the ability to install the extension's objects in
> another schema than the pg_extension default one
>  - how to provide extension author a way to have major PG version dependant
> code without having to implement and maintain a specific function in their
> install.sql file
>
> Please go comment on this other thread if you think the syntax is awful or
> for helping me through the big tickets:
>  http://archives.postgresql.org/pgsql-hackers/2009-06/msg01281.php
>
>> A decent module infrastructure is probably not going to fix this
>> problem unless it links with -ldwiw.  There are really only two
>> options here:
>
> I beg to defer. The way for a decent *extension* facility to handle the case
> is by providing an upgrade function which accepts too arguments: old and new
> version of the module. Then the module author is able to run custom code
> from within the module upgrade transaction, where migrating on disk data
> representation is entirely possible. pg_depend would have to allow for easy
> finding of a given datatype column I guess.
>
>> (I am also not aware that anyone is working on the module
>> infrastructure problem, though of course that doesn't mean that no-one
>> is; but the point is that's neither here nor there as respects the
>> present problem.  The module infrastructure is just a management layer
>> around the same underlying issues.)
>
> Of course if anyone wants to join, I'd appreciate. Some have offered help
> and I've been failing to provide them with my todo list... but getting a
> first patch for next commit fest is a goal.

This would have been a good time to start a new thread with a
different subject line.

...Robert

>>>>> "David" == "David E Wheeler" <david(at)kineticode(dot)com> writes:

>> The other option would be to fix the wasted-space bug in the
>> existing hstore, and document that stored data must be updated at
>> least once subsequent to that fix before doing a binary migrate to
>> 8.5.

[...]

David> Could it be that only folks with such a manifestation of the
David> bug would need to do a binary upgrade? If so, I certainly
David> think it'd be worth it to fix the bug.

Let's go through the options.

A)
- don't fix the wasted-space bug (or don't rely on it, anyway)
- change the new format to be more distinguishable
Result:
- seamless binary upgrade for contrib/hstore users
- users of unreleased CVS hstore-new will have to ensure all
values are updated after installing a release version and
before doing a binary upgrade to 8.5

B)
- fix the wasted-space bug
- leave the new format as-is
Result:
- seamless binary upgrade for hstore-new users
- contrib/ users will have to remove wasted space from at least
any hstore with a zero-length key before doing a binary upgrade

To me (A) is looking like the obvious choice (the people smart enough
to be using hstore-new from CVS already can handle the minor pain of
updating the on-disk format).

Unless I hear any objections I will proceed accordingly...

--
Andrew (irc:RhodiumToad)

On Jul 22, 2009, at 11:17 AM, Andrew Gierth wrote:

> To me (A) is looking like the obvious choice (the people smart enough
> to be using hstore-new from CVS already can handle the minor pain of
> updating the on-disk format).
>
> Unless I hear any objections I will proceed accordingly...

Yes, that seems like the smarter path to me, too, as long as the new
format does not continue the bug, of course.

But should the "bug" be fixed in maintenance branches? I'm thinking,
since its likelihood is so rare, probably not.

Best,

David

On Wed, Jul 22, 2009 at 2:17 PM, Andrew
Gierth<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
> Unless I hear any objections I will proceed accordingly...

At this point it's been 12 days since this was written and no updated
patch has been posted, so I think it's well past time to move this to
"Returned with Feedback". Accordingly I'm going to make that change.
Hopefully, an updated patch will be ready in time for the September
CommitFest.

Thanks,

...Robert

Tom Lane wrote:
> Perhaps an appropriate thing to do is separate out the representation
> change from the other new features, and apply just the latter for now.
> Or maybe we should think about having two versions of hstore. This
> is all tied up in the problem of having a decent module infrastructure
> (which I hope somebody is working on for 8.5). I don't know where
> we're going to end up for 8.5, but I'm disinclined to let a fairly
> minor contrib feature improvement break upgrade-compatibility before
> we've even really started the cycle.

I can just have pg_migrator detect hstore and require it be removed
before upgrading; we did that already for 8.3 to 8.4 and I am assuming
we will continue to have cases there pg_migrator just will not work.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
>
>> Perhaps an appropriate thing to do is separate out the representation
>> change from the other new features, and apply just the latter for now.
>> Or maybe we should think about having two versions of hstore. This
>> is all tied up in the problem of having a decent module infrastructure
>> (which I hope somebody is working on for 8.5). I don't know where
>> we're going to end up for 8.5, but I'm disinclined to let a fairly
>> minor contrib feature improvement break upgrade-compatibility before
>> we've even really started the cycle.
>>
>
> I can just have pg_migrator detect hstore and require it be removed
> before upgrading; we did that already for 8.3 to 8.4 and I am assuming
> we will continue to have cases there pg_migrator just will not work.
>
>

The more things you exclude the less useful the tool will be. I'm
already fairly sure it will be unusable for all or almost all my clients
who use 8.3.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> > Tom Lane wrote:
> >
> >> Perhaps an appropriate thing to do is separate out the representation
> >> change from the other new features, and apply just the latter for now.
> >> Or maybe we should think about having two versions of hstore. This
> >> is all tied up in the problem of having a decent module infrastructure
> >> (which I hope somebody is working on for 8.5). I don't know where
> >> we're going to end up for 8.5, but I'm disinclined to let a fairly
> >> minor contrib feature improvement break upgrade-compatibility before
> >> we've even really started the cycle.
> >>
> >
> > I can just have pg_migrator detect hstore and require it be removed
> > before upgrading; we did that already for 8.3 to 8.4 and I am assuming
> > we will continue to have cases there pg_migrator just will not work.
> >
> >
>
> The more things you exclude the less useful the tool will be. I'm
> already fairly sure it will be unusable for all or almost all my clients
> who use 8.3.

Sorry to hear that. You have studied the existing limitations in the
README, right?

I think it is important to report cases where pg_migrator doesn't work,
but I don't think we will ever avoid such cases. We can't stop Postgres
from moving forward, so my bet is we are always going to have such cases
where pg_migrator doesn't work.

I can't imagine losing a huge percentage of pg_migrator users via hstore
changes, especially since you can migrate hstore manually and still use
pg_migrator.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
>>>
>>> I can just have pg_migrator detect hstore and require it be removed
>>> before upgrading; we did that already for 8.3 to 8.4 and I am assuming
>>> we will continue to have cases there pg_migrator just will not work.
>>>
>>>
>>>
>> The more things you exclude the less useful the tool will be. I'm
>> already fairly sure it will be unusable for all or almost all my clients
>> who use 8.3.
>>
>
> Sorry to hear that. You have studied the existing limitations in the
> README, right?
>
> I think it is important to report cases where pg_migrator doesn't work,
> but I don't think we will ever avoid such cases. We can't stop Postgres
> from moving forward, so my bet is we are always going to have such cases
> where pg_migrator doesn't work.
>
> I can't imagine losing a huge percentage of pg_migrator users via hstore
> changes, especially since you can migrate hstore manually and still use
> pg_migrator.
>
>

We could finesse the hstore issues, but we are already blown out of the
water right now by the enum issue. My biggest end client has been
replacing small lookup tables with enums ever since they migrated to 8.3
many months ago. Another end client is currently moving to implement FTS
on 8.4, and they will be hit by the tsvector/GIN restrictions in the
future unless we fix that. All I was saying is that the more such
restrictions there are the less people will be able to use the tool.
Surely that is undeniable. I think it's great we (i.e. you) have made a
start on this, but there is a long way to go.

cheers

andrew

Andrew Dunstan wrote:
> > I can't imagine losing a huge percentage of pg_migrator users via hstore
> > changes, especially since you can migrate hstore manually and still use
> > pg_migrator.
> >
> >
>
> We could finesse the hstore issues, but we are already blown out of the
> water right now by the enum issue. My biggest end client has been
> replacing small lookup tables with enums ever since they migrated to 8.3
> many months ago. Another end client is currently moving to implement FTS

Ah, yea, enum is an issue.

> on 8.4, and they will be hit by the tsvector/GIN restrictions in the
> future unless we fix that. All I was saying is that the more such

FTS will be fine for migration from 8.4 to 8.5; it was only the
internal format change that caused FTS migration not to work from 8.3 to
8.4 (index rebuild required). There is nothing about FTS that prevents
migration.

Here is the pg_migrator README with the restrictions:

http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/pg-migrator/pg_migrator/README?rev=1.56&content-type=text/x-cvsweb-markup

I will need to document that many of these are 8.3-8.4-only migration
issues.

> restrictions there are the less people will be able to use the tool.
> Surely that is undeniable. I think it's great we (i.e. you) have made a
> start on this, but there is a long way to go.

Yes, I just don't want pg_migrator to be seen as a "complainer" and
something that is always a drag on progress. Even if we had no data
format change, using pg_migrator is still a 14-step process, so my guess
is that only people with large databases where dump/reload is
unreasonably long will use it, and in such cases, having to manually
migrate some items is probably acceptable.

What is important for me is that when pg_migrator succeeds, it is
reliable, and when it can't migrate something, it is clear.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Dunstan wrote:
> > > I can't imagine losing a huge percentage of pg_migrator users via hstore
> > > changes, especially since you can migrate hstore manually and still use
> > > pg_migrator.
> > >
> > >
> >
> > We could finesse the hstore issues, but we are already blown out of the
> > water right now by the enum issue. My biggest end client has been
> > replacing small lookup tables with enums ever since they migrated to 8.3
> > many months ago. Another end client is currently moving to implement FTS
>
> Ah, yea, enum is an issue.
>
> > on 8.4, and they will be hit by the tsvector/GIN restrictions in the
> > future unless we fix that. All I was saying is that the more such
>
> FTS will be fine for migration from 8.4 to 8.5; it was only the
> internal format change that caused FTS migration not to work from 8.3 to
> 8.4 (index rebuild required). There is nothing about FTS that prevents
> migration.
>
> Here is the pg_migrator README with the restrictions:
>
> http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/pg-migrator/pg_migrator/README?rev=1.56&content-type=text/x-cvsweb-markup
>
> I will need to document that many of these are 8.3-8.4-only migration
> issues.

Done:

http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/pg-migrator/pg_migrator/README?rev=1.57&content-type=text/x-cvsweb-markup

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Robert Haas wrote:
> On Wed, Jul 22, 2009 at 2:17 PM, Andrew
> Gierth<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>> Unless I hear any objections I will proceed accordingly...
>
> At this point it's been 12 days since this was written and no updated
> patch has been posted, so I think it's well past time to move this to
> "Returned with Feedback". Accordingly I'm going to make that change.
> Hopefully, an updated patch will be ready in time for the September
> CommitFest.

Curious if this patch is likely for 8.5 and/or if there's a newer
patch available. I've come across an application that it seems
well suited for, and would be happy to test whichever version
of the patch would be most useful for me to test against.

Ron Mayer wrote:
> Robert Haas wrote:
> > On Wed, Jul 22, 2009 at 2:17 PM, Andrew
> > Gierth<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
> >> Unless I hear any objections I will proceed accordingly...
> >
> > At this point it's been 12 days since this was written and no updated
> > patch has been posted, so I think it's well past time to move this to
> > "Returned with Feedback". Accordingly I'm going to make that change.
> > Hopefully, an updated patch will be ready in time for the September
> > CommitFest.
>
> Curious if this patch is likely for 8.5 and/or if there's a newer
> patch available. I've come across an application that it seems
> well suited for, and would be happy to test whichever version
> of the patch would be most useful for me to test against.

Yea, I was wondering about this too. I do think we should just change
the format and pg_migrator will detect the change and prevent migration
for those cases.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

>>>>> "Ron" == Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:

>> At this point it's been 12 days since this was written and no
>> updated patch has been posted, so I think it's well past time to
>> move this to "Returned with Feedback". Accordingly I'm going to
>> make that change. Hopefully, an updated patch will be ready in
>> time for the September CommitFest.

Ron> Curious if this patch is likely for 8.5 and/or if there's a
Ron> newer patch available. I've come across an application that it
Ron> seems well suited for, and would be happy to test whichever
Ron> version of the patch would be most useful for me to test
Ron> against.

I'm planning to submit another version soon.

--
Andrew (irc:RhodiumToad)