Thread: Removing pg_migrator limitations
There are several pg_migrator limitations that appeared late in the 8.4
development cycle and were impossible to fix at that point. I would
like to fix them for Postgres 8.5:
o a user-defined composite data type o a user-defined array data type o a user-defined enum data
type
I have discussed this with Alvaro. I think pg_migrator needs the
ability to set the pg_type.oid and pg_enum.oid for user-defined
composites, arrays, and enums to match the values in the old server, and
hence match references to those rows in user data tables.
The general solution will involve creating place-hold rows in pg_type
and pg_enum with the desired oids, and deleting those placeholder rows
at the time pg_dump creates the new type or enum, and passing the
desired oid to the creation command. We do something similar for toast
tables now, but it is easier there because the oids are actually file
system files.
There is no ability to specify an OID column value on insert. However,
pg_migrator has the ability to call backend C functions via shared
library functions so it could potentially insert the needed system
catalog dummy rows. As far as creating rows with the proper oids, we
could modify the SQL grammar to allow it, or modify DefineType() so it
accepts oids and passes them to TypeCreate(), or a simpler approach
would be to set the oid counter before calling CREATE TYPE, but that
would be error-prone because other oids might be assigned in
indeterminate order --- we removed that code from pg_migrator for toast
tables before 8.4 shipped, so I am not excited to re-add it. The same
approach is necessary for enums.
Another approach could be to create the dummy rows, load all of the
pg_dump schema, then renumber the rows to the proper oids, but this
assumes that I will be able to find all references to the current oids
and renumber those too.
Seems I need some help here.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > There are several pg_migrator limitations that appeared late in the 8.4 > development cycle and were impossible to fix at that point. I would > like to fix them for Postgres 8.5: > > o a user-defined composite data type > o a user-defined array data type > o a user-defined enum data type > > I have discussed this with Alvaro. I think pg_migrator needs the > ability to set the pg_type.oid and pg_enum.oid for user-defined > composites, arrays, and enums to match the values in the old server, and > hence match references to those rows in user data tables. To be more precise, the pg_enum.oid needs to be set for ENUM types; there's no need for setting the pg_type.oid (for ENUM types). I don't know about composites but I think the problem with user defined arrays is the OID of the element type, not the array itself. > The general solution will involve creating place-hold rows in pg_type > and pg_enum with the desired oids, and deleting those placeholder rows > at the time pg_dump creates the new type or enum, and passing the > desired oid to the creation command. I don't think there's a need for pg_enum placeholders. Just create them with the correct OIDs as the first step. Nobody else is going to use pg_enum.oids anyway. Again, I don't know about arrays or composites. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > There are several pg_migrator limitations that appeared late in the 8.4
> > development cycle and were impossible to fix at that point. I would
> > like to fix them for Postgres 8.5:
> >
> > o a user-defined composite data type
> > o a user-defined array data type
> > o a user-defined enum data type
> >
> > I have discussed this with Alvaro. I think pg_migrator needs the
> > ability to set the pg_type.oid and pg_enum.oid for user-defined
> > composites, arrays, and enums to match the values in the old server, and
> > hence match references to those rows in user data tables.
>
> To be more precise, the pg_enum.oid needs to be set for ENUM types;
> there's no need for setting the pg_type.oid (for ENUM types). I don't
> know about composites but I think the problem with user defined arrays
> is the OID of the element type, not the array itself.
Yes, good point. I can see where the oids are assigned in our C code:
oids[i] = GetNewOid(pg_enum);
array_oid = GetNewOid(pg_type);
I need a way of controlling that. Now, ideally, I would just be able to
add an optional oid field to DefineType() and call it from a server-side
C function called by pg_migrator, but the problem is that that function
assumes it is receiving a complex struct DefineStmt which can't easily
be created by pg_migrator.
> > The general solution will involve creating place-hold rows in pg_type
> > and pg_enum with the desired oids, and deleting those placeholder rows
> > at the time pg_dump creates the new type or enum, and passing the
> > desired oid to the creation command.
>
> I don't think there's a need for pg_enum placeholders. Just create them
> with the correct OIDs as the first step. Nobody else is going to use
> pg_enum.oids anyway. Again, I don't know about arrays or composites.
That will make things easier because of the large number of oids
consumed by enumerated types.
I am now thinking that setting the oid counter before calling CREATE
TYPE/ENUM might be the cleanest, and of course with pg_dump setting this
all up when in --binary-upgrade mode. It does make pg_migrator
dependent on the order of oid allocation in those routines. It also
might make some migrations impossible if concurrent enum creation caused
gaps in the assignment of oids in a single enumerated type.
A crazier idea would be for pg_migrator to set server-side global
variables that contain the oids to be used. pg_dump would call those
functions to set and clear the global variables when in --binary-upgrade
mode, and the backend code would consult those variables before calling
GetNewOid(), or GetNewOid() would consult those global variables.
You can now see why this was not fixed in 8.4. :-(
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Alvaro Herrera wrote: > > Bruce Momjian wrote: > > > There are several pg_migrator limitations that appeared late in the 8.4 > > > development cycle and were impossible to fix at that point. I would > > > like to fix them for Postgres 8.5: > > > > > > o a user-defined composite data type > > > o a user-defined array data type > > > o a user-defined enum data type > > > > > > I have discussed this with Alvaro. I think pg_migrator needs the > > > ability to set the pg_type.oid and pg_enum.oid for user-defined > > > composites, arrays, and enums to match the values in the old server, and > > > hence match references to those rows in user data tables. > > > > To be more precise, the pg_enum.oid needs to be set for ENUM types; > > there's no need for setting the pg_type.oid (for ENUM types). I don't > > know about composites but I think the problem with user defined arrays > > is the OID of the element type, not the array itself. > > Yes, good point. I can see where the oids are assigned in our C code: > > oids[i] = GetNewOid(pg_enum); > > array_oid = GetNewOid(pg_type); > > I need a way of controlling that. You're (partly?) missing my point which is that the important OID to control is the one that actually gets stored on table files. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Bruce Momjian wrote: > > Alvaro Herrera wrote: > > > Bruce Momjian wrote: > > > > There are several pg_migrator limitations that appeared late in the 8.4 > > > > development cycle and were impossible to fix at that point. I would > > > > like to fix them for Postgres 8.5: > > > > > > > > o a user-defined composite data type > > > > o a user-defined array data type > > > > o a user-defined enum data type > > > > > > > > I have discussed this with Alvaro. I think pg_migrator needs the > > > > ability to set the pg_type.oid and pg_enum.oid for user-defined > > > > composites, arrays, and enums to match the values in the old server, and > > > > hence match references to those rows in user data tables. > > > > > > To be more precise, the pg_enum.oid needs to be set for ENUM types; > > > there's no need for setting the pg_type.oid (for ENUM types). I don't > > > know about composites but I think the problem with user defined arrays > > > is the OID of the element type, not the array itself. > > > > Yes, good point. I can see where the oids are assigned in our C code: > > > > oids[i] = GetNewOid(pg_enum); > > > > array_oid = GetNewOid(pg_type); > > > > I need a way of controlling that. > > You're (partly?) missing my point which is that the important OID to > control is the one that actually gets stored on table files. Well, I thought the idea was to set the system table oid to match the oids already in the user tables. I realize that is not all system oids. What am I missing exactly? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Alvaro Herrera wrote: > > Bruce Momjian wrote: > > > Alvaro Herrera wrote: > > > > To be more precise, the pg_enum.oid needs to be set for ENUM types; > > > > there's no need for setting the pg_type.oid (for ENUM types). I don't > > > > know about composites but I think the problem with user defined arrays > > > > is the OID of the element type, not the array itself. > > > > > > Yes, good point. I can see where the oids are assigned in our C code: > > > > > > oids[i] = GetNewOid(pg_enum); > > > > > > array_oid = GetNewOid(pg_type); > > > > > > I need a way of controlling that. > > > > You're (partly?) missing my point which is that the important OID to > > control is the one that actually gets stored on table files. > > Well, I thought the idea was to set the system table oid to match the > oids already in the user tables. I realize that is not all system oids. > What am I missing exactly? I think the OIDs for user-defined arrays stored in table data are element types, not the array type which is what you're pointing at with the line you quote: > > > array_oid = GetNewOid(pg_type); IMBFOS. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Bruce Momjian wrote: > > Alvaro Herrera wrote: > > > Bruce Momjian wrote: > > > > Alvaro Herrera wrote: > > > > > > To be more precise, the pg_enum.oid needs to be set for ENUM types; > > > > > there's no need for setting the pg_type.oid (for ENUM types). I don't > > > > > know about composites but I think the problem with user defined arrays > > > > > is the OID of the element type, not the array itself. > > > > > > > > Yes, good point. I can see where the oids are assigned in our C code: > > > > > > > > oids[i] = GetNewOid(pg_enum); > > > > > > > > array_oid = GetNewOid(pg_type); > > > > > > > > I need a way of controlling that. > > > > > > You're (partly?) missing my point which is that the important OID to > > > control is the one that actually gets stored on table files. > > > > Well, I thought the idea was to set the system table oid to match the > > oids already in the user tables. I realize that is not all system oids. > > What am I missing exactly? > > I think the OIDs for user-defined arrays stored in table data are > element types, not the array type which is what you're pointing at with > the line you quote: > > > > > array_oid = GetNewOid(pg_type); > > IMBFOS. Oh, yea, sorry, I was just showing examples of where we get the oids --- I have not researched the exact calls yet, but I am doing that now and will apply a patch that adds C comments to the C structures to identify them. I figure it would be good to document this no matter what we do. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > There are several pg_migrator limitations that appeared late in the 8.4 > development cycle and were impossible to fix at that point. I would > like to fix them for Postgres 8.5: > > o a user-defined composite data type > o a user-defined array data type > o a user-defined enum data type > > I have discussed this with Alvaro. I think pg_migrator needs the > ability to set the pg_type.oid and pg_enum.oid for user-defined > composites, arrays, and enums to match the values in the old server, and > hence match references to those rows in user data tables. > > The general solution will involve creating place-hold rows in pg_type > and pg_enum with the desired oids, and deleting those placeholder rows > at the time pg_dump creates the new type or enum, and passing the > desired oid to the creation command. We do something similar for toast > tables now, but it is easier there because the oids are actually file > system files. > > There is no ability to specify an OID column value on insert. However, > pg_migrator has the ability to call backend C functions via shared > library functions so it could potentially insert the needed system > catalog dummy rows. As far as creating rows with the proper oids, we > could modify the SQL grammar to allow it, or modify DefineType() so it > accepts oids and passes them to TypeCreate(), or a simpler approach > would be to set the oid counter before calling CREATE TYPE, but that > would be error-prone because other oids might be assigned in > indeterminate order --- we removed that code from pg_migrator for toast > tables before 8.4 shipped, so I am not excited to re-add it. The same > approach is necessary for enums. > > Another approach could be to create the dummy rows, load all of the > pg_dump schema, then renumber the rows to the proper oids, but this > assumes that I will be able to find all references to the current oids > and renumber those too. > > Seems I need some help here. > > I thought there was a suggestion that we would be able to specify the oids in the SQL that creates the types, along the lines of: create type foo as enum ( ...) with oids ( ... ); Is that a non-starter? I imagine it would need to require some special setting to be enabled to allow it. cheers andrew
On Fri, Dec 18, 2009 at 6:44 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > I thought there was a suggestion that we would be able to specify the oids > in the SQL that creates the types, along the lines of: > > create type foo as enum ( ...) with oids ( ... ); > > Is that a non-starter? I imagine it would need to require some special > setting to be enabled to allow it. This gets at a question that I've been wondering about. There seems to be something about OIDs that makes us want to not ever allow users to specify them, or only when our back is absolutely against the wall.I have only the vaguest notions of what might be dangerousabout that, though. ...Robert
> Bruce Momjian wrote:
>> Seems I need some help here.
I'm willing to work on this --- it doesn't look particularly fun but
we really need it.
Andrew Dunstan <andrew@dunslane.net> writes:
> I thought there was a suggestion that we would be able to specify the
> oids in the SQL that creates the types, along the lines of:
> create type foo as enum ( ...) with oids ( ... );
> Is that a non-starter? I imagine it would need to require some special
> setting to be enabled to allow it.
The more I think about it the less I want such warts placed in the
regular SQL syntax for creation commands. As soon as we add a wart like
that we'll be stuck with supporting it forever. Whatever we do here
should be off in a little corner that only pg_migrator can get at.
And we already have a way to manage that: there's already something
in pg_migrator to let it install special functions that are present
only while migrating. So I suggest that we make whatever hacks are
needed available only at the C-code level, and let pg_migrator get
at them via its special functions.
In practice, this would mean teaching pg_dump to call these functions
when it is making a --binary_upgrade dump. The reason I think this
is less of a support hazard than changing SQL statements is that there
is no promise or intention that a --binary_upgrade dump will load into
anything but the specific PG version that it's intended for. (We
could, and probably should, add some version labeling to the dump to
help enforce that.)
At the moment it appears that we need the following hacks:
* ability to control the OIDs assigned to user tables and types.
Because a table also has a rowtype, this means at least two separate
state variables. And we already knew we had to control the OIDs
assigned to toast tables. I'm imagining dump output like
select pg_migrator_set_next_table_oid(123456);select pg_migrator_set_next_type_oid(12347);select
pg_migrator_set_next_toast_table_oid(123458);
CREATE TABLE ...
where the functions cause static variables to become set, and the
core code gets changed to look like
if (next_table_oid){ newoid = next_table_oid; next_table_oid = 0;}else newoid = GetNewOid(...);
in selected places where currently there's just a GetNewOid(...) call.
* ability to control the OIDs assigned to enum values. To keep this
sane I think the easiest way is to have pg_migrator have a function
that adds one value with a predetermined OID to an existing enum.
So instead of CREATE TYPE foo AS ENUM ('bar', 'baz', ...)
I envision the --binary_upgrade dump output looking like
-- force the OID of the enum type itselfselect pg_migrator_set_next_type_oid(12347);
CREATE TYPE foo AS ENUM ();
select pg_migrator_add_enum_value(12347, 'bar', 12348);select pg_migrator_add_enum_value(12347, 'baz', 12349);...
I don't see any value in the placeholder-row approach Bruce suggests;
AFAICS it would require significantly uglier backend hacks than the
above because dealing with an already-present row would be a bigger
code change.
Comments?
regards, tom lane
Tom Lane wrote:
> * ability to control the OIDs assigned to user tables and types.
> Because a table also has a rowtype, this means at least two separate
> state variables. And we already knew we had to control the OIDs
> assigned to toast tables. I'm imagining dump output like
>
> select pg_migrator_set_next_table_oid(123456);
> select pg_migrator_set_next_type_oid(12347);
> select pg_migrator_set_next_toast_table_oid(123458);
>
> CREATE TABLE ...
Do we also need a knob for the table type's array type?
> * ability to control the OIDs assigned to enum values. To keep this
> sane I think the easiest way is to have pg_migrator have a function
> that adds one value with a predetermined OID to an existing enum.
> So instead of CREATE TYPE foo AS ENUM ('bar', 'baz', ...)
> I envision the --binary_upgrade dump output looking like
>
> -- force the OID of the enum type itself
> select pg_migrator_set_next_type_oid(12347);
This part isn't necessary AFAIK, except to be used as reference here:
> CREATE TYPE foo AS ENUM ();
>
> select pg_migrator_add_enum_value(12347, 'bar', 12348);
> select pg_migrator_add_enum_value(12347, 'baz', 12349);
on which we could perhaps use "foo" as a reference instead of the OID
value. However, I think array and composite types need a specific type
OID, so the set_next_type_oid function would still be necessary.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote:
> At the moment it appears that we need the following hacks:
>
> * ability to control the OIDs assigned to user tables and types.
> Because a table also has a rowtype, this means at least two separate
> state variables. And we already knew we had to control the OIDs
> assigned to toast tables. I'm imagining dump output like
>
> select pg_migrator_set_next_table_oid(123456);
> select pg_migrator_set_next_type_oid(12347);
> select pg_migrator_set_next_toast_table_oid(123458);
>
> CREATE TABLE ...
>
> where the functions cause static variables to become set, and the
> core code gets changed to look like
>
> if (next_table_oid)
> {
> newoid = next_table_oid;
> next_table_oid = 0;
> }
> else
> newoid = GetNewOid(...);
>
> in selected places where currently there's just a GetNewOid(...) call.
>
> * ability to control the OIDs assigned to enum values. To keep this
> sane I think the easiest way is to have pg_migrator have a function
> that adds one value with a predetermined OID to an existing enum.
> So instead of CREATE TYPE foo AS ENUM ('bar', 'baz', ...)
> I envision the --binary_upgrade dump output looking like
>
> -- force the OID of the enum type itself
> select pg_migrator_set_next_type_oid(12347);
>
> CREATE TYPE foo AS ENUM ();
>
> select pg_migrator_add_enum_value(12347, 'bar', 12348);
> select pg_migrator_add_enum_value(12347, 'baz', 12349);
> ...
>
>
> I don't see any value in the placeholder-row approach Bruce suggests;
> AFAICS it would require significantly uglier backend hacks than the
> above because dealing with an already-present row would be a bigger
> code change.
>
> Comments?
>
>
>
That looks fairly workable. The placeholder idea seems like a bit of a
potential footgun, so I like the idea that we can in some limited
circumstances set the oids fairly directly.
cheers
andrew
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> select pg_migrator_set_next_table_oid(123456);
>> select pg_migrator_set_next_type_oid(12347);
>> select pg_migrator_set_next_toast_table_oid(123458);
>>
>> CREATE TABLE ...
> Do we also need a knob for the table type's array type?
Well, we wouldn't care about the oid of the array type, except that if
the backend is allowed to assign it on its own, it might eat an oid that
we're going to need later for another type. So yeah, array oids too.
(The above is just a sketch, I don't promise it's complete ;-))
>> -- force the OID of the enum type itself
>> select pg_migrator_set_next_type_oid(12347);
> This part isn't necessary AFAIK, except to be used as reference here:
>> CREATE TYPE foo AS ENUM ();
Exactly. We have to assign the oid of the enum type just as much as any
other type. Basically, to avoid collisions we'll need to ensure we nail
down the oids of every pg_class and pg_type row to be the same as they
were before. We might have to nail down relfilenodes similarly, not
sure yet.
regards, tom lane
Bruce Momjian wrote: > > I think the OIDs for user-defined arrays stored in table data are > > element types, not the array type which is what you're pointing at with > > the line you quote: > > > > > > > array_oid = GetNewOid(pg_type); > > > > IMBFOS. > > Oh, yea, sorry, I was just showing examples of where we get the oids --- > I have not researched the exact calls yet, but I am doing that now and > will apply a patch that adds C comments to the C structures to identify > them. I figure it would be good to document this no matter what we do. I have applied the attached patch which documents the locations where system oids have to be preserved for binary upgrades. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/catalog/pg_enum.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/catalog/pg_enum.c,v retrieving revision 1.9 diff -c -c -r1.9 pg_enum.c *** src/backend/catalog/pg_enum.c 1 Jan 2009 17:23:37 -0000 1.9 --- src/backend/catalog/pg_enum.c 19 Dec 2009 00:46:10 -0000 *************** *** 67,72 **** --- 67,76 ---- oids = (Oid *) palloc(n * sizeof(Oid)); for (i = 0; i < n; i++) { + /* + * The pg_enum.oid is stored in user tables. This oid must be + * preserved by binary upgrades. + */ oids[i] = GetNewOid(pg_enum); } Index: src/backend/commands/typecmds.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/commands/typecmds.c,v retrieving revision 1.139 diff -c -c -r1.139 typecmds.c *** src/backend/commands/typecmds.c 7 Dec 2009 05:22:21 -0000 1.139 --- src/backend/commands/typecmds.c 19 Dec 2009 00:46:10 -0000 *************** *** 531,536 **** --- 531,542 ---- * now have TypeCreate do all the real work. */ typoid = + /* + * The pg_type.oid is stored in user tables as array elements + * (base types) in ArrayType and in composite types in + * DatumTupleFields. This oid must be preserved by binary + * upgrades. + */ TypeCreate(InvalidOid, /* no predetermined type OID */ typeName, /* type name */ typeNamespace, /* namespace */ Index: src/backend/utils/adt/arrayfuncs.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/adt/arrayfuncs.c,v retrieving revision 1.161 diff -c -c -r1.161 arrayfuncs.c *** src/backend/utils/adt/arrayfuncs.c 4 Sep 2009 11:20:22 -0000 1.161 --- src/backend/utils/adt/arrayfuncs.c 19 Dec 2009 00:46:13 -0000 *************** *** 328,333 **** --- 328,338 ---- SET_VARSIZE(retval, nbytes); retval->ndim = ndim; retval->dataoffset = dataoffset; + /* + * This comes from the array's pg_type.typelem (which points to the + * base data type's pg_type.oid) and stores system oids in user tables. + * This oid must be preserved by binary upgrades. + */ retval->elemtype = element_type; memcpy(ARR_DIMS(retval), dim, ndim * sizeof(int)); memcpy(ARR_LBOUND(retval), lBound, ndim * sizeof(int)); Index: src/backend/utils/adt/enum.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/adt/enum.c,v retrieving revision 1.7 diff -c -c -r1.7 enum.c *** src/backend/utils/adt/enum.c 1 Jan 2009 17:23:49 -0000 1.7 --- src/backend/utils/adt/enum.c 19 Dec 2009 00:46:13 -0000 *************** *** 56,61 **** --- 56,65 ---- format_type_be(enumtypoid), name))); + /* + * This comes from pg_enum.oid and stores system oids in user tables. + * This oid must be preserved by binary upgrades. + */ enumoid = HeapTupleGetOid(tup); ReleaseSysCache(tup); Index: src/backend/utils/adt/rowtypes.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/adt/rowtypes.c,v retrieving revision 1.25 diff -c -c -r1.25 rowtypes.c *** src/backend/utils/adt/rowtypes.c 11 Jun 2009 14:49:04 -0000 1.25 --- src/backend/utils/adt/rowtypes.c 19 Dec 2009 00:46:14 -0000 *************** *** 97,102 **** --- 97,107 ---- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("input of anonymous composite types is not implemented"))); tupTypmod = -1; /* for all non-anonymous types */ + /* + * This comes from the composite type's pg_type.oid and + * stores system oids in user tables, specifically DatumTupleFields. + * This oid must be preserved by binary upgrades. + */ tupdesc = lookup_rowtype_tupdesc(tupType, tupTypmod); ncolumns = tupdesc->natts;
Tom Lane wrote:
> The more I think about it the less I want such warts placed in the
> regular SQL syntax for creation commands. As soon as we add a wart like
> that we'll be stuck with supporting it forever. Whatever we do here
> should be off in a little corner that only pg_migrator can get at.
Yea, and we might need more some day so a system that can be easily
enhanced would help. Adding to SQL syntax and maintaining it seems like
overkill.
> And we already have a way to manage that: there's already something
> in pg_migrator to let it install special functions that are present
> only while migrating. So I suggest that we make whatever hacks are
> needed available only at the C-code level, and let pg_migrator get
> at them via its special functions.
Right.
> In practice, this would mean teaching pg_dump to call these functions
> when it is making a --binary_upgrade dump. The reason I think this
> is less of a support hazard than changing SQL statements is that there
> is no promise or intention that a --binary_upgrade dump will load into
> anything but the specific PG version that it's intended for. (We
> could, and probably should, add some version labeling to the dump to
> help enforce that.)
Yea, that is easy.
> At the moment it appears that we need the following hacks:
>
> * ability to control the OIDs assigned to user tables and types.
> Because a table also has a rowtype, this means at least two separate
> state variables. And we already knew we had to control the OIDs
> assigned to toast tables. I'm imagining dump output like
>
> select pg_migrator_set_next_table_oid(123456);
> select pg_migrator_set_next_type_oid(12347);
> select pg_migrator_set_next_toast_table_oid(123458);
I was thinking of something even more general:
select pg_migrator_set_oid('pg_type', 100);select pg_migrator_set_oid('pg_type_array', 101);
and you just check for the strings in pg_migrator_set_oid and set the
proper variable. The idea I had was to create a global structure:
struct pg_migrator_oids { Oid pg_type; Oid pg_type_array; ...}
This would initialize to zero as a global structure, and only
pg_migrator server-side functions set it.
> CREATE TABLE ...
>
> where the functions cause static variables to become set, and the
> core code gets changed to look like
>
> if (next_table_oid)
> {
> newoid = next_table_oid;
> next_table_oid = 0;
> }
> else
> newoid = GetNewOid(...);
Yes, that is what I was thinking too:
if (pg_migrator_oid.pg_type) { newoid = pg_migrator_oid.pg_type; pg_migrator_oid.pg_type = 0;
}else newoid = GetNewOid(...);
> in selected places where currently there's just a GetNewOid(...) call.
>
> * ability to control the OIDs assigned to enum values. To keep this
> sane I think the easiest way is to have pg_migrator have a function
> that adds one value with a predetermined OID to an existing enum.
> So instead of CREATE TYPE foo AS ENUM ('bar', 'baz', ...)
> I envision the --binary_upgrade dump output looking like
>
> -- force the OID of the enum type itself
> select pg_migrator_set_next_type_oid(12347);
>
> CREATE TYPE foo AS ENUM ();
>
> select pg_migrator_add_enum_value(12347, 'bar', 12348);
> select pg_migrator_add_enum_value(12347, 'baz', 12349);
> ...
Good idea --- I was trying to figure out how to assign an array of oids
and couldn't think of a simple way.
> I don't see any value in the placeholder-row approach Bruce suggests;
> AFAICS it would require significantly uglier backend hacks than the
> above because dealing with an already-present row would be a bigger
> code change.
True.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Tom Lane wrote: > >> select pg_migrator_set_next_table_oid(123456); > >> select pg_migrator_set_next_type_oid(12347); > >> select pg_migrator_set_next_toast_table_oid(123458); > >> > >> CREATE TABLE ... > > > Do we also need a knob for the table type's array type? > > Well, we wouldn't care about the oid of the array type, except that if > the backend is allowed to assign it on its own, it might eat an oid that > we're going to need later for another type. So yeah, array oids too. > (The above is just a sketch, I don't promise it's complete ;-)) > > >> -- force the OID of the enum type itself > >> select pg_migrator_set_next_type_oid(12347); > > > This part isn't necessary AFAIK, except to be used as reference here: > > >> CREATE TYPE foo AS ENUM (); > > Exactly. We have to assign the oid of the enum type just as much as any > other type. Basically, to avoid collisions we'll need to ensure we nail > down the oids of every pg_class and pg_type row to be the same as they I assume you meant pg_type and pg_class above, or I hope you were. > were before. We might have to nail down relfilenodes similarly, not > sure yet. Yea, piggybacking on Alvaro's idea for pg_enum, if we set all the pg_type oids we can clearly do this with no placeholders necessary. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 12/18/2009 04:09 PM, Tom Lane wrote:
> At the moment it appears that we need the following hacks:
>
> * ability to control the OIDs assigned to user tables and types.
> Because a table also has a rowtype, this means at least two separate
> state variables. And we already knew we had to control the OIDs
> assigned to toast tables. I'm imagining dump output like
>
> select pg_migrator_set_next_table_oid(123456);
> select pg_migrator_set_next_type_oid(12347);
> select pg_migrator_set_next_toast_table_oid(123458);
>
> CREATE TABLE ...
I like this approach overall, but wonder if it would be better to do:
select pg_migrator_set_next_oid('table', 123456);select pg_migrator_set_next_oid('type', 12347);select
pg_migrator_set_next_oid('toast_table',123458);
etc. Later we could easily add other supported objects...
Joe
Bruce Momjian <bruce@momjian.us> writes:
> ... The idea I had was to create a global structure:
> struct pg_migrator_oids {
> Oid pg_type;
> Oid pg_type_array;
> ...
> }
> This would initialize to zero as a global structure, and only
> pg_migrator server-side functions set it.
I would prefer *not* to do that, as that makes the list of settable oids
far more public than I would like; also you are totally dependent on
pg_migrator and the backend to be in sync about the definition of that
struct, which is going to be problematic in alpha releases in
particular, since PG_VERSION isn't going to distinguish them.
What I had in mind was more like
static Oid next_pg_class_oid = InvalidOid;
voidset_next_pg_class_oid(Oid oid){ next_pg_class_oid = oid;}
in each module that needs to be able to accept a next-oid setting,
and then the pg_migrator loadable module would expose SQL-callable
wrappers for these functions. That way, any inconsistency shows up as
a link error: function needed not present.
regards, tom lane
Joe Conway <mail@joeconway.com> writes:
> I like this approach overall, but wonder if it would be better to do:
> select pg_migrator_set_next_oid('table', 123456);
> select pg_migrator_set_next_oid('type', 12347);
> select pg_migrator_set_next_oid('toast_table', 123458);
> etc. Later we could easily add other supported objects...
Yeah, Bruce was just suggesting the same. I do like that part of what
he mentioned, just because it'll be fewer special functions to add and
drop in pg_migrator.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > ... The idea I had was to create a global structure:
>
> > struct pg_migrator_oids {
> > Oid pg_type;
> > Oid pg_type_array;
> > ...
> > }
>
> > This would initialize to zero as a global structure, and only
> > pg_migrator server-side functions set it.
>
> I would prefer *not* to do that, as that makes the list of settable oids
> far more public than I would like; also you are totally dependent on
> pg_migrator and the backend to be in sync about the definition of that
> struct, which is going to be problematic in alpha releases in
> particular, since PG_VERSION isn't going to distinguish them.
>
> What I had in mind was more like
>
> static Oid next_pg_class_oid = InvalidOid;
>
> void
> set_next_pg_class_oid(Oid oid)
> {
> next_pg_class_oid = oid;
> }
Good point about requiring a link to a symbol; a structure offset would
not link to anything and would silently fail.
Does exporting a function buy us anything vs. exporting a variable?
> in each module that needs to be able to accept a next-oid setting,
> and then the pg_migrator loadable module would expose SQL-callable
> wrappers for these functions. That way, any inconsistency shows up as
> a link error: function needed not present.
I will work on a patch to accomplish this, and have pg_migrator link in
the .so only if the new server is >= 8.5, which allows a single
pg_migrator binary to work for migration to 8.4 and 8.5.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > > Bruce Momjian wrote: > >> Seems I need some help here. > > I'm willing to work on this --- it doesn't look particularly fun but > we really need it. You don't know fun until you have tried to stack hack upon hack and still create a reliable migration system. :-( -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sat, Dec 19, 2009 at 10:46 PM, Bruce Momjian <bruce@momjian.us> wrote: > Tom Lane wrote: >> > Bruce Momjian wrote: >> >> Seems I need some help here. >> >> I'm willing to work on this --- it doesn't look particularly fun but >> we really need it. > > You don't know fun until you have tried to stack hack upon hack and > still create a reliable migration system. :-( They say that people who love sausage and respect the law should never watch either one being made, and I have to say I'm coming to feel that way about in-place upgrade, too. ...Robert
Robert Haas wrote: > On Sat, Dec 19, 2009 at 10:46 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Tom Lane wrote: > >> > Bruce Momjian wrote: > >> >> Seems I need some help here. > >> > >> I'm willing to work on this --- it doesn't look particularly fun but > >> we really need it. > > > > You don't know fun until you have tried to stack hack upon hack and > > still create a reliable migration system. ?:-( > > They say that people who love sausage and respect the law should never > watch either one being made, and I have to say I'm coming to feel that > way about in-place upgrade, too. Agreed ... There is nothing to see here --- move along. ;-) LOL -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> What I had in mind was more like
>>
>> static Oid next_pg_class_oid = InvalidOid;
>>
>> void
>> set_next_pg_class_oid(Oid oid)
>> {
>> next_pg_class_oid = oid;
>> }
> Does exporting a function buy us anything vs. exporting a variable?
Hmm, probably not. I generally like to avoid global variables, but
in this case it doesn't seem to buy us anything to do so. Actually,
you could just have the core code do
/* don't make this static, pg_migrator needs to set it */Oid next_pg_class_oid = InvalidOid;
and not even bother with an extern declaration in the backend header
files (AFAIK gcc won't complain about that). That would help keep the
variable private to just the one core module plus pg_migrator.
> I will work on a patch to accomplish this, and have pg_migrator link in
> the .so only if the new server is >= 8.5, which allows a single
> pg_migrator binary to work for migration to 8.4 and 8.5.
I think you're just creating useless work for yourself by imagining that
pg_migrator is backend-version-independent. In fact, I was thinking
about proposing that we pull it in as a contrib module. Because so much
of what it does is tied to details of backend and pg_dump behavior, it's
just a pipe dream to think that developing it as a separate project is
helpful.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> What I had in mind was more like
> >>
> >> static Oid next_pg_class_oid = InvalidOid;
> >>
> >> void
> >> set_next_pg_class_oid(Oid oid)
> >> {
> >> next_pg_class_oid = oid;
> >> }
>
> > Does exporting a function buy us anything vs. exporting a variable?
>
> Hmm, probably not. I generally like to avoid global variables, but
> in this case it doesn't seem to buy us anything to do so. Actually,
> you could just have the core code do
>
> /* don't make this static, pg_migrator needs to set it */
> Oid next_pg_class_oid = InvalidOid;
>
> and not even bother with an extern declaration in the backend header
> files (AFAIK gcc won't complain about that). That would help keep the
> variable private to just the one core module plus pg_migrator.
Yes, that was the idea. We wouldn't expose the variable in other C
files unless necessary.
> > I will work on a patch to accomplish this, and have pg_migrator link in
> > the .so only if the new server is >= 8.5, which allows a single
> > pg_migrator binary to work for migration to 8.4 and 8.5.
>
> I think you're just creating useless work for yourself by imagining that
> pg_migrator is backend-version-independent. In fact, I was thinking
> about proposing that we pull it in as a contrib module. Because so much
> of what it does is tied to details of backend and pg_dump behavior, it's
> just a pipe dream to think that developing it as a separate project is
> helpful.
Well, I do think it will work for 8.3 to 8.4 ,and 8.4 to 8.5 --- I test
that regularly and I have not seen any failures in that regard. If we
move it into /contrib, I will make sure fixes get backpatched. FYI, a
typical test is:
if (GET_MAJOR_VERSION(ctx.old.pg_version) <= 803 && GET_MAJOR_VERSION(ctx.new.pg_version) >= 804)
The biggest issue with versions is that it is hard for pg_migrator to
text changes in the server during major development so for example once
we add pg_dump support for system oids, it will then require CVS HEAD
for 8.5, but there are not that many people testing pg_migrator with
non-HEAD versions.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I think you're just creating useless work for yourself by imagining that
>> pg_migrator is backend-version-independent. In fact, I was thinking
>> about proposing that we pull it in as a contrib module. Because so much
>> of what it does is tied to details of backend and pg_dump behavior, it's
>> just a pipe dream to think that developing it as a separate project is
>> helpful.
> Well, I do think it will work for 8.3 to 8.4 ,and 8.4 to 8.5 --- I test
> that regularly and I have not seen any failures in that regard. If we
> move it into /contrib, I will make sure fixes get backpatched. FYI, a
> typical test is:
> if (GET_MAJOR_VERSION(ctx.old.pg_version) <= 803 &&
> GET_MAJOR_VERSION(ctx.new.pg_version) >= 804)
Well, yeah, you can probably make it work if you're willing to carry
enoguh version tests and alternate sets of logic in the source code.
I don't think that is a particularly good engineering approach however.
It makes things less readable and probably more buggy. Particularly
so since we are talking about some quite significant logic changes here.
There's a reason to clutter, eg, pg_dump with multiple version support.
I don't see the argument for doing so with pg_migrator. Separate source
code branches seem like a much better idea.
regards, tom lane
On Sun, Dec 20, 2009 at 1:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> if (GET_MAJOR_VERSION(ctx.old.pg_version) <= 803 && >> GET_MAJOR_VERSION(ctx.new.pg_version) >= 804) > > Well, yeah, you can probably make it work if you're willing to carry > enoguh version tests and alternate sets of logic in the source code. > I don't think that is a particularly good engineering approach however. > It makes things less readable and probably more buggy. Particularly > so since we are talking about some quite significant logic changes here. > > There's a reason to clutter, eg, pg_dump with multiple version support. > I don't see the argument for doing so with pg_migrator. Separate source > code branches seem like a much better idea. I guess we have to look at the specific cases that come up, but ISTM that a branch here amounts to a second copy of the code that has to be separately maintained. I'm having a hard time working up much enthusiasm for that prospect. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Dec 20, 2009 at 1:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There's a reason to clutter, eg, pg_dump with multiple version support.
>> I don't see the argument for doing so with pg_migrator. �Separate source
>> code branches seem like a much better idea.
> I guess we have to look at the specific cases that come up, but ISTM
> that a branch here amounts to a second copy of the code that has to be
> separately maintained. I'm having a hard time working up much
> enthusiasm for that prospect.
Well, it'd be exactly like back-patching fixes across multiple branches
is for fixes in the core code now. In code that hasn't changed across
branches, that's not terribly painful. If the code has changed, then
you're going to have pain no matter which way you do it.
But the real problem with having pg_migrator be a separate project is
that a lot of the time "fix pg_migrator" is really going to mean "fix
pg_dump" or even "change the backend". We already had problems with
version skew between pg_migrator and various 8.4 alpha/beta releases.
That type of problem isn't likely to magically go away in the future.
regards, tom lane
On Sun, Dec 20, 2009 at 2:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, Dec 20, 2009 at 1:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> There's a reason to clutter, eg, pg_dump with multiple version support. >>> I don't see the argument for doing so with pg_migrator. Separate source >>> code branches seem like a much better idea. > >> I guess we have to look at the specific cases that come up, but ISTM >> that a branch here amounts to a second copy of the code that has to be >> separately maintained. I'm having a hard time working up much >> enthusiasm for that prospect. > > Well, it'd be exactly like back-patching fixes across multiple branches > is for fixes in the core code now. In code that hasn't changed across > branches, that's not terribly painful. If the code has changed, then > you're going to have pain no matter which way you do it. > > But the real problem with having pg_migrator be a separate project is > that a lot of the time "fix pg_migrator" is really going to mean "fix > pg_dump" or even "change the backend". We already had problems with > version skew between pg_migrator and various 8.4 alpha/beta releases. > That type of problem isn't likely to magically go away in the future. I agree that pulling pg_migrator into contrib seems pretty sensible. What I want to make sure we're on the same page about is which versions the 8.5 pg_migrator will allow you to upgrade from and to. I think we should at least support 8.3 -> 8.5 and 8.4 -> 8.5. If you're saying we don't need to support 8.3 -> 8.4 any more once 8.5 comes out, I'm probably OK with that, but perhaps we should try to get a few more opinions before setting that policy in stone. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> I agree that pulling pg_migrator into contrib seems pretty sensible.
> What I want to make sure we're on the same page about is which
> versions the 8.5 pg_migrator will allow you to upgrade from and to. I
> think we should at least support 8.3 -> 8.5 and 8.4 -> 8.5. If you're
> saying we don't need to support 8.3 -> 8.4 any more once 8.5 comes
> out, I'm probably OK with that, but perhaps we should try to get a few
> more opinions before setting that policy in stone.
If we can do that reasonably (which might well be the case), I'd be for
it. What I'm objecting to is what I take to be Bruce's plan of
supporting 8.3 -> 8.4 and 8.4 -> 8.5 with the same pg_migrator sources.
The stuff we're talking about doing in this thread is going to cause
those two cases to diverge rather drastically. 8.3 -> 8.5 and 8.4 ->
8.5 may be close enough together to be reasonable to support in one
set of source code.
regards, tom lane
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > I agree that pulling pg_migrator into contrib seems pretty sensible. > > What I want to make sure we're on the same page about is which > > versions the 8.5 pg_migrator will allow you to upgrade from and to. I > > think we should at least support 8.3 -> 8.5 and 8.4 -> 8.5. If you're > > saying we don't need to support 8.3 -> 8.4 any more once 8.5 comes > > out, I'm probably OK with that, but perhaps we should try to get a few > > more opinions before setting that policy in stone. > > If we can do that reasonably (which might well be the case), I'd be for > it. What I'm objecting to is what I take to be Bruce's plan of > supporting 8.3 -> 8.4 and 8.4 -> 8.5 with the same pg_migrator sources. > The stuff we're talking about doing in this thread is going to cause > those two cases to diverge rather drastically. 8.3 -> 8.5 and 8.4 -> > 8.5 may be close enough together to be reasonable to support in one > set of source code. Basically there isn't much extra work to go from 8.3 to 8.4 compared to 8.3 to 8.5. Now, if could support only 8.4 to 8.5 I could remove some code, but that seems counterproductive. The other problem with moving to /contrib is that I can't put out pg_migrator updates independently of the main community release, which could be bad. I am glad some people think pg_migrator is ready for /contrib. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sun, 20 Dec 2009, Bruce Momjian wrote: > The other problem with moving to /contrib is that I can't put out > pg_migrator updates independently of the main community release, which > could be bad. Why not? ---- Marc G. Fournier Hub.Org Hosting Solutions S.A. scrappy@hub.org http://www.hub.org Yahoo:yscrappy Skype: hub.org ICQ:7615664 MSN:scrappy@hub.org
Bruce Momjian <bruce@momjian.us> writes:
> Basically there isn't much extra work to go from 8.3 to 8.4 compared to
> 8.3 to 8.5.
That might be true today, but it will stop being true once we replace
the oid/relfilenode management hac^Wcode with the proposed new approach.
> The other problem with moving to /contrib is that I can't put out
> pg_migrator updates independently of the main community release, which
> could be bad.
That was a good thing while pg_migrator was in its "wild west"
development stage. But if you have ambitions that people should trust it
enough to risk their production DBs on it, then it had better be stable
enough for this not to be a big drawback.
Also note the point about how it'll be a lot easier to keep it in sync
with pg_dump and backend behavior if it's only got to work with the
pg_dump version that's in the same release. Again, the proposed changes
tie it to a particular pg_dump and target backend version noticeably
more than it was before; so if you try to keep it separate this is going
to be an even bigger headache than it already was during the run-up to
8.4.
Lastly, getting pg_migrator working reliably would be a sufficiently
Big Deal that I think a critical pg_migrator bug would be sufficient
grounds for forcing a minor release, just as we sometimes force a
release for critical backend bugs.
> I am glad some people think pg_migrator is ready for /contrib.
To be clear, I don't think it's really ready for contrib today. I think
it could be up to that level by the time 8.5 releases, but we have to
get serious about it, and stop pretending it's an arm's-length project
the core project doesn't really care about.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Basically there isn't much extra work to go from 8.3 to 8.4 compared to > > 8.3 to 8.5. > > That might be true today, but it will stop being true once we replace > the oid/relfilenode management hac^Wcode with the proposed new approach. > > > The other problem with moving to /contrib is that I can't put out > > pg_migrator updates independently of the main community release, which > > could be bad. > > That was a good thing while pg_migrator was in its "wild west" > development stage. But if you have ambitions that people should trust it > enough to risk their production DBs on it, then it had better be stable > enough for this not to be a big drawback. > > Also note the point about how it'll be a lot easier to keep it in sync > with pg_dump and backend behavior if it's only got to work with the > pg_dump version that's in the same release. Again, the proposed changes > tie it to a particular pg_dump and target backend version noticeably > more than it was before; so if you try to keep it separate this is going > to be an even bigger headache than it already was during the run-up to > 8.4. > > Lastly, getting pg_migrator working reliably would be a sufficiently > Big Deal that I think a critical pg_migrator bug would be sufficient > grounds for forcing a minor release, just as we sometimes force a > release for critical backend bugs. > > > I am glad some people think pg_migrator is ready for /contrib. > > To be clear, I don't think it's really ready for contrib today. I think > it could be up to that level by the time 8.5 releases, but we have to > get serious about it, and stop pretending it's an arm's-length project > the core project doesn't really care about. OK, I am convinced. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Dec 19, 2009, at 9:52 PM, Robert Haas wrote: > On Sat, Dec 19, 2009 at 10:46 PM, Bruce Momjian <bruce@momjian.us> wrote: >> Tom Lane wrote: >>>> Bruce Momjian wrote: >>>>> Seems I need some help here. >>> >>> I'm willing to work on this --- it doesn't look particularly fun but >>> we really need it. >> >> You don't know fun until you have tried to stack hack upon hack and >> still create a reliable migration system. :-( > > They say that people who love sausage and respect the law should never > watch either one being made, and I have to say I'm coming to feel that > way about in-place upgrade, too. Perhaps we should be ordering bacon instead of sausage... Is there some reason why OIDs were used for ENUM instead of a general sequence? Were we worried about people screwing withthe sequence? A sequences would presumably eliminate all these issues. Even if we wanted to disallow user access to the sequence, havingsomething that's not competing with all the other uses of OID would presumably make this a lot simpler. -- Jim C. Nasby, Database Architect jim@nasby.net 512.569.9461 (cell) http://jim.nasby.net
Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > ... The idea I had was to create a global structure:
> >
> > > struct pg_migrator_oids {
> > > Oid pg_type;
> > > Oid pg_type_array;
> > > ...
> > > }
> >
> > > This would initialize to zero as a global structure, and only
> > > pg_migrator server-side functions set it.
> >
> > I would prefer *not* to do that, as that makes the list of settable oids
> > far more public than I would like; also you are totally dependent on
> > pg_migrator and the backend to be in sync about the definition of that
> > struct, which is going to be problematic in alpha releases in
> > particular, since PG_VERSION isn't going to distinguish them.
> >
> > What I had in mind was more like
> >
> > static Oid next_pg_class_oid = InvalidOid;
> >
> > void
> > set_next_pg_class_oid(Oid oid)
> > {
> > next_pg_class_oid = oid;
> > }
>
> Good point about requiring a link to a symbol; a structure offset would
> not link to anything and would silently fail.
>
> Does exporting a function buy us anything vs. exporting a variable?
>
> > in each module that needs to be able to accept a next-oid setting,
> > and then the pg_migrator loadable module would expose SQL-callable
> > wrappers for these functions. That way, any inconsistency shows up as
> > a link error: function needed not present.
>
> I will work on a patch to accomplish this, and have pg_migrator link in
> the .so only if the new server is >= 8.5, which allows a single
> pg_migrator binary to work for migration to 8.4 and 8.5.
I have completed the attached patch which assigns oids for all pg_type
rows when pg_dump --binary-upgrade is used. This allows user-defined
arrays and composite types to be migrated cleanly. I tested a reload of
the regression database with --binary-upgrade and all the pg_type oids
were identical. The pg_migrator changes required to use this feature
are trivial.
The remaining issue is pg_enum oids. Because it will be difficult to
pass an arbitrary number of oids into the backend, the idea was to
assign each enum value separately. If we implement this TODO item:
Allow adding/renaming/removing enumerated values to an existing
enumerated data type
Particularly the "adding" part rather than the "renaming/removing" part,
pg_dump can create an enum type with one (or zero perhaps) enums, and
then use a pg_enum oid-setting function and then use ALTER TYPE ADD
ENUM to add each new value.
Comments?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.361
diff -c -c -r1.361 heap.c
*** src/backend/catalog/heap.c 7 Dec 2009 05:22:21 -0000 1.361
--- src/backend/catalog/heap.c 23 Dec 2009 18:48:11 -0000
***************
*** 1001,1013 ****
if (IsUnderPostmaster && (relkind == RELKIND_RELATION ||
relkind == RELKIND_VIEW ||
relkind == RELKIND_COMPOSITE_TYPE))
! {
! /* OK, so pre-assign a type OID for the array type */
! Relation pg_type = heap_open(TypeRelationId, AccessShareLock);
!
! new_array_oid = GetNewOid(pg_type);
! heap_close(pg_type, AccessShareLock);
! }
/*
* Since defining a relation also defines a complex type, we add a new
--- 1001,1007 ----
if (IsUnderPostmaster && (relkind == RELKIND_RELATION ||
relkind == RELKIND_VIEW ||
relkind == RELKIND_COMPOSITE_TYPE))
! new_array_oid = AssignTypeArrayOid();
/*
* Since defining a relation also defines a complex type, we add a new
Index: src/backend/catalog/pg_type.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/pg_type.c,v
retrieving revision 1.127
diff -c -c -r1.127 pg_type.c
*** src/backend/catalog/pg_type.c 16 Aug 2009 18:14:34 -0000 1.127
--- src/backend/catalog/pg_type.c 23 Dec 2009 18:48:11 -0000
***************
*** 32,37 ****
--- 32,38 ----
#include "utils/rel.h"
#include "utils/syscache.h"
+ Oid binary_upgrade_next_pg_type_oid = InvalidOid;
/* ----------------------------------------------------------------
* TypeShellMake
***************
*** 119,124 ****
--- 120,131 ----
*/
tup = heap_form_tuple(tupDesc, values, nulls);
+ if (OidIsValid(binary_upgrade_next_pg_type_oid))
+ {
+ HeapTupleSetOid(tup, binary_upgrade_next_pg_type_oid);
+ binary_upgrade_next_pg_type_oid = InvalidOid;
+ }
+
/*
* insert the tuple in the relation and get the tuple's oid.
*/
***************
*** 409,418 ****
values,
nulls);
! /* Force the OID if requested by caller, else heap_insert does it */
if (OidIsValid(newTypeOid))
HeapTupleSetOid(tup, newTypeOid);
!
typeObjectId = simple_heap_insert(pg_type_desc, tup);
}
--- 416,431 ----
values,
nulls);
! /* Force the OID if requested by caller */
if (OidIsValid(newTypeOid))
HeapTupleSetOid(tup, newTypeOid);
! else if (OidIsValid(binary_upgrade_next_pg_type_oid))
! {
! HeapTupleSetOid(tup, binary_upgrade_next_pg_type_oid);
! binary_upgrade_next_pg_type_oid = InvalidOid;
! }
! /* else allow system to assign oid */
!
typeObjectId = simple_heap_insert(pg_type_desc, tup);
}
Index: src/backend/catalog/toasting.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/toasting.c,v
retrieving revision 1.22
diff -c -c -r1.22 toasting.c
*** src/backend/catalog/toasting.c 23 Dec 2009 02:35:18 -0000 1.22
--- src/backend/catalog/toasting.c 23 Dec 2009 18:48:11 -0000
***************
*** 31,36 ****
--- 31,37 ----
#include "utils/builtins.h"
#include "utils/syscache.h"
+ Oid binary_upgrade_next_pg_type_toast_oid = InvalidOid;
static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
Datum reloptions, bool force);
***************
*** 121,126 ****
--- 122,128 ----
Relation class_rel;
Oid toast_relid;
Oid toast_idxid;
+ Oid toast_typid = InvalidOid;
Oid namespaceid;
char toast_relname[NAMEDATALEN];
char toast_idxname[NAMEDATALEN];
***************
*** 199,209 ****
else
namespaceid = PG_TOAST_NAMESPACE;
toast_relid = heap_create_with_catalog(toast_relname,
namespaceid,
rel->rd_rel->reltablespace,
toastOid,
! InvalidOid,
rel->rd_rel->relowner,
tupdesc,
NIL,
--- 201,217 ----
else
namespaceid = PG_TOAST_NAMESPACE;
+ if (OidIsValid(binary_upgrade_next_pg_type_toast_oid))
+ {
+ toast_typid = binary_upgrade_next_pg_type_toast_oid;
+ binary_upgrade_next_pg_type_toast_oid = InvalidOid;
+ }
+
toast_relid = heap_create_with_catalog(toast_relname,
namespaceid,
rel->rd_rel->reltablespace,
toastOid,
! toast_typid,
rel->rd_rel->relowner,
tupdesc,
NIL,
Index: src/backend/commands/typecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/typecmds.c,v
retrieving revision 1.140
diff -c -c -r1.140 typecmds.c
*** src/backend/commands/typecmds.c 19 Dec 2009 00:47:57 -0000 1.140
--- src/backend/commands/typecmds.c 23 Dec 2009 18:48:11 -0000
***************
*** 74,79 ****
--- 74,80 ----
/* atts[] is of allocated length RelationGetNumberOfAttributes(rel) */
} RelToCheck;
+ Oid binary_upgrade_next_pg_type_array_oid = InvalidOid;
static Oid findTypeInputFunction(List *procname, Oid typeOid);
static Oid findTypeOutputFunction(List *procname, Oid typeOid);
***************
*** 143,149 ****
Oid array_oid;
Oid typoid;
Oid resulttype;
- Relation pg_type;
ListCell *pl;
/*
--- 144,149 ----
***************
*** 522,531 ****
NameListToString(analyzeName));
#endif
! /* Preassign array type OID so we can insert it in pg_type.typarray */
! pg_type = heap_open(TypeRelationId, AccessShareLock);
! array_oid = GetNewOid(pg_type);
! heap_close(pg_type, AccessShareLock);
/*
* now have TypeCreate do all the real work.
--- 522,528 ----
NameListToString(analyzeName));
#endif
! array_oid = AssignTypeArrayOid();
/*
* now have TypeCreate do all the real work.
***************
*** 1101,1107 ****
AclResult aclresult;
Oid old_type_oid;
Oid enumArrayOid;
- Relation pg_type;
/* Convert list of names to a name and namespace */
enumNamespace = QualifiedNameGetCreationNamespace(stmt->typeName,
--- 1098,1103 ----
***************
*** 1129,1138 ****
errmsg("type \"%s\" already exists", enumName)));
}
! /* Preassign array type OID so we can insert it in pg_type.typarray */
! pg_type = heap_open(TypeRelationId, AccessShareLock);
! enumArrayOid = GetNewOid(pg_type);
! heap_close(pg_type, AccessShareLock);
/* Create the pg_type entry */
enumTypeOid =
--- 1125,1131 ----
errmsg("type \"%s\" already exists", enumName)));
}
! enumArrayOid = AssignTypeArrayOid();
/* Create the pg_type entry */
enumTypeOid =
***************
*** 1470,1475 ****
--- 1463,1495 ----
return procOid;
}
+ /*
+ * AssignTypeArrayOid
+ *
+ * Pre-assign the type's array OID for use in pg_type.typarray
+ */
+ Oid
+ AssignTypeArrayOid(void)
+ {
+ Oid type_array_oid;
+
+ /* Pre-assign the type's array OID for use in pg_type.typarray */
+ if (OidIsValid(binary_upgrade_next_pg_type_array_oid))
+ {
+ type_array_oid = binary_upgrade_next_pg_type_array_oid;
+ binary_upgrade_next_pg_type_array_oid = InvalidOid;
+ }
+ else
+ {
+ Relation pg_type = heap_open(TypeRelationId, AccessShareLock);
+
+ type_array_oid = GetNewOid(pg_type);
+ heap_close(pg_type, AccessShareLock);
+ }
+
+ return type_array_oid;
+ }
+
/*-------------------------------------------------------------------
* DefineCompositeType
Index: src/bin/pg_dump/pg_dump.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.560
diff -c -c -r1.560 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 23 Dec 2009 04:10:50 -0000 1.560
--- src/bin/pg_dump/pg_dump.c 23 Dec 2009 18:48:12 -0000
***************
*** 196,201 ****
--- 196,206 ----
static void dumpDatabase(Archive *AH);
static void dumpEncoding(Archive *AH);
static void dumpStdStrings(Archive *AH);
+ static void binary_upgrade_set_type_oids_by_type_oid(
+ PQExpBuffer upgrade_buffer, Oid pg_type_oid);
+ static bool binary_upgrade_set_type_oids_by_rel_oid(
+ PQExpBuffer upgrade_buffer, Oid pg_rel_oid);
+ static void binary_upgrade_clear_pg_type_toast_oid(PQExpBuffer upgrade_buffer);
static const char *getAttrName(int attrnum, TableInfo *tblInfo);
static const char *fmtCopyColumnList(const TableInfo *ti);
static void do_sql_command(PGconn *conn, const char *query);
***************
*** 2176,2181 ****
--- 2181,2311 ----
return 1;
}
+ static void
+ binary_upgrade_set_type_oids_by_type_oid(PQExpBuffer upgrade_buffer,
+ Oid pg_type_oid)
+ {
+ PQExpBuffer upgrade_query = createPQExpBuffer();
+ int ntups;
+ PGresult *upgrade_res;
+ Oid pg_type_array_oid;
+
+ appendPQExpBuffer(upgrade_buffer, "\n-- For binary upgrade, must preserve pg_type oid\n");
+ appendPQExpBuffer(upgrade_buffer,
+ "SELECT binary_upgrade.set_next_pg_type_oid('%u'::pg_catalog.oid);\n\n",
+ pg_type_oid);
+
+ /* we only support old >= 8.3 for binary upgrades */
+ appendPQExpBuffer(upgrade_query,
+ "SELECT typarray "
+ "FROM pg_catalog.pg_type "
+ "WHERE pg_type.oid = '%u'::pg_catalog.oid;",
+ pg_type_oid);
+
+ upgrade_res = PQexec(g_conn, upgrade_query->data);
+ check_sql_result(upgrade_res, g_conn, upgrade_query->data, PGRES_TUPLES_OK);
+
+ /* Expecting a single result only */
+ ntups = PQntuples(upgrade_res);
+ if (ntups != 1)
+ {
+ write_msg(NULL, ngettext("query returned %d row instead of one: %s\n",
+ "query returned %d rows instead of one: %s\n",
+ ntups),
+ ntups, upgrade_query->data);
+ exit_nicely();
+ }
+
+ pg_type_array_oid = atooid(PQgetvalue(upgrade_res, 0, PQfnumber(upgrade_res, "typarray")));
+
+ if (OidIsValid(pg_type_array_oid))
+ {
+ appendPQExpBuffer(upgrade_buffer,
+ "\n-- For binary upgrade, must preserve pg_type array oid\n");
+ appendPQExpBuffer(upgrade_buffer,
+ "SELECT binary_upgrade.set_next_pg_type_array_oid('%u'::pg_catalog.oid);\n\n",
+ pg_type_array_oid);
+ }
+
+ PQclear(upgrade_res);
+ destroyPQExpBuffer(upgrade_query);
+ }
+
+ static bool
+ binary_upgrade_set_type_oids_by_rel_oid(PQExpBuffer upgrade_buffer,
+ Oid pg_rel_oid)
+ {
+ PQExpBuffer upgrade_query = createPQExpBuffer();
+ int ntups;
+ PGresult *upgrade_res;
+ Oid pg_type_oid;
+ bool toast_set = false;
+
+ /* we only support old >= 8.3 for binary upgrades */
+ appendPQExpBuffer(upgrade_query,
+ "SELECT c.reltype AS crel, t.reltype AS trel "
+ "FROM pg_catalog.pg_class c "
+ "LEFT JOIN pg_catalog.pg_class t ON "
+ " (c.reltoastrelid = t.oid) "
+ "WHERE c.oid = '%u'::pg_catalog.oid;",
+ pg_rel_oid);
+
+ upgrade_res = PQexec(g_conn, upgrade_query->data);
+ check_sql_result(upgrade_res, g_conn, upgrade_query->data, PGRES_TUPLES_OK);
+
+ /* Expecting a single result only */
+ ntups = PQntuples(upgrade_res);
+ if (ntups != 1)
+ {
+ write_msg(NULL, ngettext("query returned %d row instead of one: %s\n",
+ "query returned %d rows instead of one: %s\n",
+ ntups),
+ ntups, upgrade_query->data);
+ exit_nicely();
+ }
+
+ pg_type_oid = atooid(PQgetvalue(upgrade_res, 0, PQfnumber(upgrade_res, "crel")));
+
+ binary_upgrade_set_type_oids_by_type_oid(upgrade_buffer, pg_type_oid);
+
+ if (!PQgetisnull(upgrade_res, 0, PQfnumber(upgrade_res, "trel")))
+ {
+ /* Toast tables do not have pg_type array rows */
+ Oid pg_type_toast_oid = atooid(PQgetvalue(upgrade_res, 0,
+ PQfnumber(upgrade_res, "trel")));
+
+ appendPQExpBuffer(upgrade_buffer, "\n-- For binary upgrade, must preserve pg_type toast oid\n");
+ appendPQExpBuffer(upgrade_buffer,
+ "SELECT binary_upgrade.set_next_pg_type_toast_oid('%u'::pg_catalog.oid);\n\n",
+ pg_type_toast_oid);
+
+ toast_set = true;
+ }
+
+ PQclear(upgrade_res);
+ destroyPQExpBuffer(upgrade_query);
+
+ return toast_set;
+ }
+
+ static void
+ binary_upgrade_clear_pg_type_toast_oid(PQExpBuffer upgrade_buffer)
+ {
+ /*
+ * One complexity is that while the heap might now have a TOAST table,
+ * the TOAST table might have been created long after creation when
+ * the table was loaded with wide data. For that reason, we clear
+ * binary_upgrade_set_next_pg_type_toast_oid so it is not reused
+ * by a later table. Logically any later creation that needs a TOAST
+ * table should have its own TOAST pg_type oid, but we are cautious.
+ */
+ appendPQExpBuffer(upgrade_buffer,
+ "\n-- For binary upgrade, clear toast oid because it might not have been needed\n");
+ appendPQExpBuffer(upgrade_buffer,
+ "SELECT binary_upgrade.set_next_pg_type_oid('%u'::pg_catalog.oid);\n\n",
+ InvalidOid);
+ }
+
/*
* getNamespaces:
* read all namespaces in the system catalogs and return them in the
***************
*** 6428,6433 ****
--- 6558,6567 ----
fmtId(tyinfo->dobj.namespace->dobj.name));
appendPQExpBuffer(delq, "%s;\n",
fmtId(tyinfo->dobj.name));
+
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid);
+
appendPQExpBuffer(q, "CREATE TYPE %s AS ENUM (\n",
fmtId(tyinfo->dobj.name));
for (i = 0; i < num; i++)
***************
*** 6723,6728 ****
--- 6857,6866 ----
appendPQExpBuffer(delq, "%s CASCADE;\n",
fmtId(tyinfo->dobj.name));
+ /* We might already have a shell type, but setting pg_type_oid is harmless */
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid);
+
appendPQExpBuffer(q,
"CREATE TYPE %s (\n"
" INTERNALLENGTH = %s",
***************
*** 6892,6897 ****
--- 7030,7038 ----
else
typdefault = NULL;
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid);
+
appendPQExpBuffer(q,
"CREATE DOMAIN %s AS %s",
fmtId(tyinfo->dobj.name),
***************
*** 7002,7007 ****
--- 7143,7151 ----
i_attname = PQfnumber(res, "attname");
i_atttypdefn = PQfnumber(res, "atttypdefn");
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid);
+
appendPQExpBuffer(q, "CREATE TYPE %s AS (",
fmtId(tyinfo->dobj.name));
***************
*** 7191,7196 ****
--- 7335,7344 ----
* after it's filled in, otherwise the backend complains.
*/
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_type_oid(q,
+ stinfo->baseType->dobj.catId.oid);
+
appendPQExpBuffer(q, "CREATE TYPE %s;\n",
fmtId(stinfo->dobj.name));
***************
*** 10226,10235 ****
char *storage;
int j,
k;
!
/* Make sure we are in proper schema */
selectSourceSchema(tbinfo->dobj.namespace->dobj.name);
/* Is it a table or a view? */
if (tbinfo->relkind == RELKIND_VIEW)
{
--- 10374,10388 ----
char *storage;
int j,
k;
! bool toast_set = false;
!
/* Make sure we are in proper schema */
selectSourceSchema(tbinfo->dobj.namespace->dobj.name);
+ if (binary_upgrade)
+ toast_set = binary_upgrade_set_type_oids_by_rel_oid(q,
+ tbinfo->dobj.catId.oid);
+
/* Is it a table or a view? */
if (tbinfo->relkind == RELKIND_VIEW)
{
***************
*** 10606,10611 ****
--- 10759,10767 ----
}
}
+ if (binary_upgrade && toast_set)
+ binary_upgrade_clear_pg_type_toast_oid(q);
+
ArchiveEntry(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
tbinfo->dobj.name,
tbinfo->dobj.namespace->dobj.name,
***************
*** 10617,10622 ****
--- 10773,10779 ----
tbinfo->dobj.dependencies, tbinfo->dobj.nDeps,
NULL, NULL);
+
/* Dump Table Comments */
dumpTableComment(fout, tbinfo, reltypename);
***************
*** 11235,11240 ****
--- 11392,11401 ----
fmtId(tbinfo->dobj.name));
resetPQExpBuffer(query);
+
+ if (binary_upgrade)
+ binary_upgrade_set_type_oids_by_rel_oid(query, tbinfo->dobj.catId.oid);
+
appendPQExpBuffer(query,
"CREATE SEQUENCE %s\n",
fmtId(tbinfo->dobj.name));
***************
*** 11270,11275 ****
--- 11431,11438 ----
appendPQExpBuffer(query, ";\n");
+ /* binary_upgrade: no need to clear TOAST table oid */
+
ArchiveEntry(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
tbinfo->dobj.name,
tbinfo->dobj.namespace->dobj.name,
Index: src/include/commands/typecmds.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/typecmds.h,v
retrieving revision 1.25
diff -c -c -r1.25 typecmds.h
*** src/include/commands/typecmds.h 1 Jan 2009 17:23:58 -0000 1.25
--- src/include/commands/typecmds.h 23 Dec 2009 18:48:12 -0000
***************
*** 25,30 ****
--- 25,31 ----
extern void DefineDomain(CreateDomainStmt *stmt);
extern void DefineEnum(CreateEnumStmt *stmt);
extern Oid DefineCompositeType(const RangeVar *typevar, List *coldeflist);
+ extern Oid AssignTypeArrayOid(void);
extern void AlterDomainDefault(List *names, Node *defaultRaw);
extern void AlterDomainNotNull(List *names, bool notNull);
decibel <decibel@decibel.org> writes:
> Is there some reason why OIDs were used for ENUM instead of a general sequence? Were we worried about people screwing
withthe sequence?
No, we were worried about being able to do enum_out without outside
information as to which enum type it is. If you don't mind doubling
the on-disk size of enum values, we could store the enum type OID and
a sequence number instead.
> A sequences would presumably eliminate all these issues. Even if we wanted to disallow user access to the sequence,
havingsomething that's not competing with all the other uses of OID would presumably make this a lot simpler.
The fact that it's shared with other uses of OID is 100% not relevant.
A counter shared across all enums would pose the same issues. The
only way to simplify matters would be to have each enum have its own
value numbering, which would mean you need outside information to
identify the associated label.
Even if there were a really solid argument for changing this decision,
doing so would create on-disk compatibility problems that would be
even harder for pg_migrator to fix than what we're discussing now.
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes:
> The remaining issue is pg_enum oids. Because it will be difficult to
> pass an arbitrary number of oids into the backend, the idea was to
> assign each enum value separately. If we implement this TODO item:
> Allow adding/renaming/removing enumerated values to an existing
> enumerated data type
The reason that isn't implemented is that it's *hard* --- in fact,
it appears to be entirely impossible in the general case, unless you're
willing to change existing values of the enum on-disk. I do not agree
that it's a good plan to try to solve that as a prerequisite to making
pg_migrator work.
regards, tom lane
On Wed, Dec 23, 2009 at 7:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Bruce Momjian <bruce@momjian.us> writes: >> The remaining issue is pg_enum oids. Because it will be difficult to >> pass an arbitrary number of oids into the backend, the idea was to >> assign each enum value separately. If we implement this TODO item: > >> Allow adding/renaming/removing enumerated values to an existing >> enumerated data type > > The reason that isn't implemented is that it's *hard* --- in fact, > it appears to be entirely impossible in the general case, unless you're > willing to change existing values of the enum on-disk. I do not agree > that it's a good plan to try to solve that as a prerequisite to making > pg_migrator work. Shouldn't adding new ones be easy? As long as we're willing to make it fail with an error if there's a conflict -- which is sufficient for pg_dump's needs. -- greg
Greg Stark <gsstark@mit.edu> writes:
> On Wed, Dec 23, 2009 at 7:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The reason that isn't implemented is that it's *hard* --- in fact,
>> it appears to be entirely impossible in the general case, unless you're
>> willing to change existing values of the enum on-disk.
> Shouldn't adding new ones be easy?
No, not if you care about where they end up in the type's sort ordering.
In pg_migrator's case that's not an issue because it's going to force
the OID numbering for each of the elements. However, an ADD ENUM VALUE
option that *doesn't* use a predetermined OID is going to end up
inserting the new value at a not-very-predictable place. I do not think
we should expose a half-baked behavior like that as standard SQL syntax.
If we're going to implement something whose ambitions only extend to
satisfying pg_migrator's needs, then it should be a specialized
pg_migrator function.
regards, tom lane
On Wed, Dec 23, 2009 at 8:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If we're going to implement something whose ambitions only extend to > satisfying pg_migrator's needs, then it should be a specialized > pg_migrator function. Fwiw my feeling was the opposite here. It's better to offer even limited SQL-level support for features pg_migrator needs because the more abstract and loosely coupled the interface is between pg_migrator and the internals the better. Even if the interface is somewhat limited and just good enough for pg_migrator's needs it's still easier to support a well-defined abstract interface than one that depends on knowing about the internal implementation. I can see I'm outvoted here though and you and Bruce are the ones writing the code so far... -- greg
Greg Stark <gsstark@mit.edu> writes:
> On Wed, Dec 23, 2009 at 8:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we're going to implement something whose ambitions only extend to
>> satisfying pg_migrator's needs, then it should be a specialized
>> pg_migrator function.
> Fwiw my feeling was the opposite here. It's better to offer even
> limited SQL-level support for features pg_migrator needs because the
> more abstract and loosely coupled the interface is between pg_migrator
> and the internals the better. Even if the interface is somewhat
> limited and just good enough for pg_migrator's needs it's still easier
> to support a well-defined abstract interface than one that depends on
> knowing about the internal implementation.
The problem is that we *don't* want a nice abstract interface. We want
one that lets us specify the exact OIDs to use for the enum values.
Which is about as non-abstract as you can get.
regards, tom lane
Tom Lane wrote: > Greg Stark <gsstark@mit.edu> writes: > > On Wed, Dec 23, 2009 at 7:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> The reason that isn't implemented is that it's *hard* --- in fact, > >> it appears to be entirely impossible in the general case, unless you're > >> willing to change existing values of the enum on-disk. > > > Shouldn't adding new ones be easy? > > No, not if you care about where they end up in the type's sort ordering. > > In pg_migrator's case that's not an issue because it's going to force > the OID numbering for each of the elements. However, an ADD ENUM VALUE > option that *doesn't* use a predetermined OID is going to end up > inserting the new value at a not-very-predictable place. I do not think > we should expose a half-baked behavior like that as standard SQL syntax. > If we're going to implement something whose ambitions only extend to > satisfying pg_migrator's needs, then it should be a specialized > pg_migrator function. I looked at DefineEnum() and basically adding the ability to add enums would put the new enum after the existing ones unless the OID counter has wrapped around and is less than the oid counter at the time the enum type was created, in which case it will be listed as before the existing values. I wasn't aware enum ordering is something we tried to maintain. One issue is that we are not supporting the addition of enum values even for people who don't care about the ordering of enums (which I bet might be the majority.) I can think of a few approaches for pg_migrator: 1) Create an oid array in a permanent memory context and have DefineEnum() read from that.2) Renumber the enum entriesafter they are created but before any of their oids are stored in user tables. Both can be done by pg_dump with proper server-side functions. The problem with #2 are cases where the old and new oid ranges overlap, e.g.: 1 2 3 becomes: 2 3 4 In that case, you can't just renumber because of oid collisions that would invalidate the oid index on pg_enum. Even the ordering of renumbering might not be consistent, e.g.: old 1 2 3 12 13 14 new 2 3 4 11 12 13 Starting renumbering from the front or back would both fail. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > I wasn't aware enum ordering is something we tried to maintain. > One issue is that we are not supporting the addition of enum values even > for people who don't care about the ordering of enums (which I bet might > be the majority.) > The ordering of enums is defined and to be relied on and I think it's absolutely unacceptable not to be able to rely on the ordering. We should never be in a position where the values returned by enum_first(), enum_range() etc. are not completely deterministic. Part of the original motivation for implementing enums was precisely so that they would sort in the defined order rather than in lexicographical order. It's a fundamental part of the type and not an optional feature. The idea of potentially breaking it makes no more sense than allowing for a non-deterministic ordering of integers. cheers andrew
Andrew Dunstan wrote: > > > Bruce Momjian wrote: > > I wasn't aware enum ordering is something we tried to maintain. > > One issue is that we are not supporting the addition of enum values even > > for people who don't care about the ordering of enums (which I bet might > > be the majority.) > > > > The ordering of enums is defined and to be relied on and I think it's > absolutely unacceptable not to be able to rely on the ordering. > > We should never be in a position where the values returned by > enum_first(), enum_range() etc. are not completely deterministic. I had no idea we exposed that API. > Part of the original motivation for implementing enums was precisely so > that they would sort in the defined order rather than in lexicographical > order. It's a fundamental part of the type and not an optional feature. > The idea of potentially breaking it makes no more sense than allowing > for a non-deterministic ordering of integers. OK, I get the point. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > I looked at DefineEnum() and basically adding the ability to add enums > would put the new enum after the existing ones unless the OID counter > has wrapped around and is less than the oid counter at the time the enum > type was created, in which case it will be listed as before the existing > values. I wasn't aware enum ordering is something we tried to maintain. > One issue is that we are not supporting the addition of enum values even > for people who don't care about the ordering of enums (which I bet might > be the majority.) > > I can think of a few approaches for pg_migrator: > > 1) Create an oid array in a permanent memory context and have > DefineEnum() read from that. > 2) Renumber the enum entries after they are created but before > any of their oids are stored in user tables. > > Both can be done by pg_dump with proper server-side functions. The > problem with #2 are cases where the old and new oid ranges overlap, > e.g.: I now think the easiest solution will be to have pg_dump create the enum with a single dummy value, delete the pg_enum dummy row, and then call a modified version of EnumValuesCreate() to insert row by row into pg_enum, with specified oids. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Bruce Momjian wrote: > > I looked at DefineEnum() and basically adding the ability to add enums > > would put the new enum after the existing ones unless the OID counter > > has wrapped around and is less than the oid counter at the time the enum > > type was created, in which case it will be listed as before the existing > > values. I wasn't aware enum ordering is something we tried to maintain. > > One issue is that we are not supporting the addition of enum values even > > for people who don't care about the ordering of enums (which I bet might > > be the majority.) > > > > I can think of a few approaches for pg_migrator: > > > > 1) Create an oid array in a permanent memory context and have > > DefineEnum() read from that. > > 2) Renumber the enum entries after they are created but before > > any of their oids are stored in user tables. > > > > Both can be done by pg_dump with proper server-side functions. The > > problem with #2 are cases where the old and new oid ranges overlap, > > e.g.: > > I now think the easiest solution will be to have pg_dump create the enum > with a single dummy value, delete the pg_enum dummy row, and then call a > modified version of EnumValuesCreate() to insert row by row into > pg_enum, with specified oids. I thought of a cleaner approach. CREATE TYPE ENUM will create one enum with the specified oid, and then a server-side function will call EnumValuesCreate() be used to add each additional enum with a specified oid --- no deleting necessary. I will start working on a patch for this. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> I thought of a cleaner approach. CREATE TYPE ENUM will create one enum
> with the specified oid, and then a server-side function will call
> EnumValuesCreate() be used to add each additional enum with a specified
> oid --- no deleting necessary. I will start working on a patch for
> this.
The approach I originally suggested was to create the enum type with
*no* members, and then add the values one at a time. It might take a
tweak to the CREATE TYPE AS ENUM grammar to allow zero members, but
I don't see any logical problem with such a thing.
regards, tom lane
Bruce Momjian wrote: >> I now think the easiest solution will be to have pg_dump create the enum >> with a single dummy value, delete the pg_enum dummy row, and then call a >> modified version of EnumValuesCreate() to insert row by row into >> pg_enum, with specified oids. >> > > I thought of a cleaner approach. CREATE TYPE ENUM will create one enum > with the specified oid, and then a server-side function will call > EnumValuesCreate() be used to add each additional enum with a specified > oid --- no deleting necessary. I will start working on a patch for > this. > > Either that or Tom's suggested approach of being able to create an empty enum type would be much cleaner than the dummy row suggestion. cheers andrew
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I thought of a cleaner approach. CREATE TYPE ENUM will create one enum
> > with the specified oid, and then a server-side function will call
> > EnumValuesCreate() be used to add each additional enum with a specified
> > oid --- no deleting necessary. I will start working on a patch for
> > this.
>
> The approach I originally suggested was to create the enum type with
> *no* members, and then add the values one at a time. It might take a
> tweak to the CREATE TYPE AS ENUM grammar to allow zero members, but
> I don't see any logical problem with such a thing.
Well, I was hesitant to modify the grammar, unless we want the ability
to create enums with zero values. Doing enum with only one value will
not be too complex for me and I don't think binary upgrade should affect
the grammar unless there are other reasons we want to change. I think
it will look like:
-- For binary upgrade, must preserve pg_enum oidsSELECT binary_upgrade.set_next_pg_enum_oid('27258'::pg_catalog.oid);
CREATE TYPE empstatus AS ENUM('hired');
SELECT binary_upgrade.set_next_pg_enum_oid('27259'::pg_catalog.oid);
SELECT binary_upgrade.add_pg_enum_value('42143'::pg_catalog.oid, 'retired');
We do allow tables with no columns, but we allow the addition of columns
to a table, so it makes more sense there.
As far as the ability to add enum values using ALTER TYPE, it seems we
would need a pg_enum.enumnum column like we do for pg_attribute.attnum
and order on that rather than pg_enum.oid. (Binary upgrade would still
need to preserve oids.)
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > I have completed the attached patch which assigns oids for all pg_type > rows when pg_dump --binary-upgrade is used. This allows user-defined > arrays and composite types to be migrated cleanly. I tested a reload of > the regression database with --binary-upgrade and all the pg_type oids > were identical. The pg_migrator changes required to use this feature > are trivial. Applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> The approach I originally suggested was to create the enum type with
>> *no* members, and then add the values one at a time.
> Well, I was hesitant to modify the grammar, unless we want the ability
> to create enums with zero values. Doing enum with only one value will
> not be too complex for me and I don't think binary upgrade should affect
> the grammar unless there are other reasons we want to change.
The reason I don't want to do it that way is that then you need two
ugly kluges in the backend, not just one. With the zero-and-add-one
approach there is no need to have a "next enum oid" variable at all.
> We do allow tables with no columns, but we allow the addition of columns
> to a table, so it makes more sense there.
Well, we might eventually allow addition of values to enums too; the
fact that it's not implemented outside pg_migrator right now doesn't
mean we won't ever think of a solution. In any case I'm not persuaded
that a zero-element enum is totally without value. Think of it like a
domain with a "must be null" constraint.
regards, tom lane
Bruce Momjian wrote: > As far as the ability to add enum values using ALTER TYPE, it seems we > would need a pg_enum.enumnum column like we do for pg_attribute.attnum > and order on that rather than pg_enum.oid. (Binary upgrade would still > need to preserve oids.) > > I don't that's necessarily a good way to go - being able to sort by the actual stored value is an efficiency point. I think we might need to look at implementing a more extensible enum type, which would allow new values to be appended to and possibly inserted into the list of labels, but anyway that's really a separate subject from pg_migrator. cheers andrew
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> The approach I originally suggested was to create the enum type with > >> *no* members, and then add the values one at a time. > > > Well, I was hesitant to modify the grammar, unless we want the ability > > to create enums with zero values. Doing enum with only one value will > > not be too complex for me and I don't think binary upgrade should affect > > the grammar unless there are other reasons we want to change. > > The reason I don't want to do it that way is that then you need two > ugly kluges in the backend, not just one. With the zero-and-add-one > approach there is no need to have a "next enum oid" variable at all. Uh, I still need that variable because that is how we are going to set the oid in EnumValuesCreate(), unless we want to add dummy oid-value arguments to that function for use only by the binary upgrade server-side function. I have actually coded the variable case already so you can see how it looks; attached. Most of the patch is just indenting of the existing oid assignment block. > > We do allow tables with no columns, but we allow the addition of columns > > to a table, so it makes more sense there. > > Well, we might eventually allow addition of values to enums too; the > fact that it's not implemented outside pg_migrator right now doesn't > mean we won't ever think of a solution. In any case I'm not persuaded > that a zero-element enum is totally without value. Think of it like a > domain with a "must be null" constraint. OK, but that is going to expand the my patch. I will probably implement zero-element enums first and then go ahead and do the binary upgrade part. Zero-element enums will simplify the pg_dump code. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/catalog/pg_enum.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/catalog/pg_enum.c,v retrieving revision 1.11 diff -c -c -r1.11 pg_enum.c *** src/backend/catalog/pg_enum.c 24 Dec 2009 22:17:58 -0000 1.11 --- src/backend/catalog/pg_enum.c 24 Dec 2009 22:29:17 -0000 *************** *** 25,30 **** --- 25,32 ---- static int oid_cmp(const void *p1, const void *p2); + Oid binary_upgrade_next_pg_enum_oid = InvalidOid; + /* * EnumValuesCreate *************** *** 58,82 **** tupDesc = pg_enum->rd_att; /* ! * Allocate oids. While this method does not absolutely guarantee that we ! * generate no duplicate oids (since we haven't entered each oid into the ! * table before allocating the next), trouble could only occur if the oid ! * counter wraps all the way around before we finish. Which seems ! * unlikely. */ oids = (Oid *) palloc(num_elems * sizeof(Oid)); ! for (elemno = 0; elemno < num_elems; elemno++) { /* ! * The pg_enum.oid is stored in user tables. This oid must be ! * preserved by binary upgrades. */ ! oids[elemno] = GetNewOid(pg_enum); } - /* sort them, just in case counter wrapped from high to low */ - qsort(oids, num_elems, sizeof(Oid), oid_cmp); - /* and make the entries */ memset(nulls, false, sizeof(nulls)); --- 60,94 ---- tupDesc = pg_enum->rd_att; /* ! * Allocate oids */ oids = (Oid *) palloc(num_elems * sizeof(Oid)); ! if (num_elems == 1 && OidIsValid(binary_upgrade_next_pg_enum_oid)) ! { ! oids[0] = binary_upgrade_next_pg_enum_oid; ! binary_upgrade_next_pg_enum_oid = InvalidOid; ! } ! else { /* ! * While this method does not absolutely guarantee that we generate ! * no duplicate oids (since we haven't entered each oid into the ! * table before allocating the next), trouble could only occur if ! * the oid counter wraps all the way around before we finish. Which ! * seems unlikely. */ ! for (elemno = 0; elemno < num_elems; elemno++) ! { ! /* ! * The pg_enum.oid is stored in user tables. This oid must be ! * preserved by binary upgrades. ! */ ! oids[elemno] = GetNewOid(pg_enum); ! } ! /* sort them, just in case counter wrapped from high to low */ ! qsort(oids, num_elems, sizeof(Oid), oid_cmp); } /* and make the entries */ memset(nulls, false, sizeof(nulls));
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> The reason I don't want to do it that way is that then you need two
>> ugly kluges in the backend, not just one. With the zero-and-add-one
>> approach there is no need to have a "next enum oid" variable at all.
> Uh, I still need that variable because that is how we are going to set
> the oid in EnumValuesCreate(), unless we want to add dummy oid-value
> arguments to that function for use only by the binary upgrade
> server-side function.
Please go back and re-read what I suggested: you need a function along
the lines ofadd_enum_member(enum-type, 'value name', value-oid)
and then there's no need for any saved state. So what if it has a
different signature from the other pg_migrator special functions?
It's not doing the same thing.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> The reason I don't want to do it that way is that then you need two > >> ugly kluges in the backend, not just one. With the zero-and-add-one > >> approach there is no need to have a "next enum oid" variable at all. > > > Uh, I still need that variable because that is how we are going to set > > the oid in EnumValuesCreate(), unless we want to add dummy oid-value > > arguments to that function for use only by the binary upgrade > > server-side function. > > Please go back and re-read what I suggested: you need a function along > the lines of > add_enum_member(enum-type, 'value name', value-oid) > and then there's no need for any saved state. So what if it has a > different signature from the other pg_migrator special functions? > It's not doing the same thing. OK, right, I can get rid of the enum function that just sets the next oid value if I do all the enum value creation via function calls. I will work in that direction then. -- Bruce Momjian <bruce@momjian.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: > > Bruce Momjian <bruce@momjian.us> writes: > > > Tom Lane wrote: > > >> The reason I don't want to do it that way is that then you need two > > >> ugly kluges in the backend, not just one. With the zero-and-add-one > > >> approach there is no need to have a "next enum oid" variable at all. > > > > > Uh, I still need that variable because that is how we are going to set > > > the oid in EnumValuesCreate(), unless we want to add dummy oid-value > > > arguments to that function for use only by the binary upgrade > > > server-side function. > > > > Please go back and re-read what I suggested: you need a function along > > the lines of > > add_enum_member(enum-type, 'value name', value-oid) > > and then there's no need for any saved state. So what if it has a > > different signature from the other pg_migrator special functions? > > It's not doing the same thing. > > OK, right, I can get rid of the enum function that just sets the next > oid value if I do all the enum value creation via function calls. I > will work in that direction then. There is only one call to EnumValuesCreate() so maybe adding a binary-upgrade-only parameter to the function will be the cleanest approach. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > Well, we might eventually allow addition of values to enums too; the > > fact that it's not implemented outside pg_migrator right now doesn't > > mean we won't ever think of a solution. In any case I'm not persuaded > > that a zero-element enum is totally without value. Think of it like a > > domain with a "must be null" constraint. > > OK, but that is going to expand the my patch. I will probably implement > zero-element enums first and then go ahead and do the binary upgrade > part. Zero-element enums will simplify the pg_dump code. I have implemented the zero-value option to CREATE TYPE ENUM with the attached patch. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/ref/create_type.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/ref/create_type.sgml,v retrieving revision 1.79 diff -c -c -r1.79 create_type.sgml *** doc/src/sgml/ref/create_type.sgml 30 Nov 2008 19:01:29 -0000 1.79 --- doc/src/sgml/ref/create_type.sgml 25 Dec 2009 06:15:56 -0000 *************** *** 25,31 **** ( <replaceable class="PARAMETER">attribute_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>[, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> AS ENUM ! ( '<replaceable class="parameter">label</replaceable>' [, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> ( INPUT = <replaceable class="parameter">input_function</replaceable>, --- 25,31 ---- ( <replaceable class="PARAMETER">attribute_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>[, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> AS ENUM ! ( [ '<replaceable class="parameter">label</replaceable>' [, ... ] ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> ( INPUT = <replaceable class="parameter">input_function</replaceable>, Index: src/backend/parser/gram.y =================================================================== RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v retrieving revision 2.699 diff -c -c -r2.699 gram.y *** src/backend/parser/gram.y 23 Dec 2009 17:41:43 -0000 2.699 --- src/backend/parser/gram.y 25 Dec 2009 06:15:56 -0000 *************** *** 297,303 **** TableFuncElementList opt_type_modifiers prep_type_clause execute_param_clause using_clause returning_clause ! enum_val_list table_func_column_list create_generic_options alter_generic_options relation_expr_list dostmt_opt_list --- 297,303 ---- TableFuncElementList opt_type_modifiers prep_type_clause execute_param_clause using_clause returning_clause ! opt_enum_val_list enum_val_list table_func_column_list create_generic_options alter_generic_options relation_expr_list dostmt_opt_list *************** *** 3623,3629 **** n->coldeflist = $6; $$ = (Node *)n; } ! | CREATE TYPE_P any_name AS ENUM_P '(' enum_val_list ')' { CreateEnumStmt *n = makeNode(CreateEnumStmt); n->typeName = $3; --- 3623,3629 ---- n->coldeflist = $6; $$ = (Node *)n; } ! | CREATE TYPE_P any_name AS ENUM_P '(' opt_enum_val_list ')' { CreateEnumStmt *n = makeNode(CreateEnumStmt); n->typeName = $3; *************** *** 3715,3720 **** --- 3715,3725 ---- } ; + opt_enum_val_list: + enum_val_list { $$ = $1; } + | /*EMPTY*/ { $$ = NIL; } + ; + enum_val_list: Sconst { $$ = list_make1(makeString($1)); } | enum_val_list ',' Sconst
Bruce Momjian wrote: > Bruce Momjian wrote: > > > Well, we might eventually allow addition of values to enums too; the > > > fact that it's not implemented outside pg_migrator right now doesn't > > > mean we won't ever think of a solution. In any case I'm not persuaded > > > that a zero-element enum is totally without value. Think of it like a > > > domain with a "must be null" constraint. > > > > OK, but that is going to expand the my patch. I will probably implement > > zero-element enums first and then go ahead and do the binary upgrade > > part. Zero-element enums will simplify the pg_dump code. > > I have implemented the zero-value option to CREATE TYPE ENUM with the > attached patch. pg_dump also needs a minor edit for this, which will appear in the oid assignment patch. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Bruce Momjian wrote: > > > Well, we might eventually allow addition of values to enums too; the > > > fact that it's not implemented outside pg_migrator right now doesn't > > > mean we won't ever think of a solution. In any case I'm not persuaded > > > that a zero-element enum is totally without value. Think of it like a > > > domain with a "must be null" constraint. > > > > OK, but that is going to expand the my patch. I will probably implement > > zero-element enums first and then go ahead and do the binary upgrade > > part. Zero-element enums will simplify the pg_dump code. > > I have implemented the zero-value option to CREATE TYPE ENUM with the > attached patch. Applied, with pg_dump support too; updated patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/ref/create_type.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/ref/create_type.sgml,v retrieving revision 1.79 diff -c -c -r1.79 create_type.sgml *** doc/src/sgml/ref/create_type.sgml 30 Nov 2008 19:01:29 -0000 1.79 --- doc/src/sgml/ref/create_type.sgml 26 Dec 2009 16:50:35 -0000 *************** *** 25,31 **** ( <replaceable class="PARAMETER">attribute_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>[, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> AS ENUM ! ( '<replaceable class="parameter">label</replaceable>' [, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> ( INPUT = <replaceable class="parameter">input_function</replaceable>, --- 25,31 ---- ( <replaceable class="PARAMETER">attribute_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>[, ... ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> AS ENUM ! ( [ '<replaceable class="parameter">label</replaceable>' [, ... ] ] ) CREATE TYPE <replaceable class="parameter">name</replaceable> ( INPUT = <replaceable class="parameter">input_function</replaceable>, Index: src/backend/parser/gram.y =================================================================== RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v retrieving revision 2.699 diff -c -c -r2.699 gram.y *** src/backend/parser/gram.y 23 Dec 2009 17:41:43 -0000 2.699 --- src/backend/parser/gram.y 26 Dec 2009 16:50:35 -0000 *************** *** 297,303 **** TableFuncElementList opt_type_modifiers prep_type_clause execute_param_clause using_clause returning_clause ! enum_val_list table_func_column_list create_generic_options alter_generic_options relation_expr_list dostmt_opt_list --- 297,303 ---- TableFuncElementList opt_type_modifiers prep_type_clause execute_param_clause using_clause returning_clause ! opt_enum_val_list enum_val_list table_func_column_list create_generic_options alter_generic_options relation_expr_list dostmt_opt_list *************** *** 3623,3629 **** n->coldeflist = $6; $$ = (Node *)n; } ! | CREATE TYPE_P any_name AS ENUM_P '(' enum_val_list ')' { CreateEnumStmt *n = makeNode(CreateEnumStmt); n->typeName = $3; --- 3623,3629 ---- n->coldeflist = $6; $$ = (Node *)n; } ! | CREATE TYPE_P any_name AS ENUM_P '(' opt_enum_val_list ')' { CreateEnumStmt *n = makeNode(CreateEnumStmt); n->typeName = $3; *************** *** 3715,3720 **** --- 3715,3725 ---- } ; + opt_enum_val_list: + enum_val_list { $$ = $1; } + | /*EMPTY*/ { $$ = NIL; } + ; + enum_val_list: Sconst { $$ = list_make1(makeString($1)); } | enum_val_list ',' Sconst Index: src/bin/pg_dump/pg_dump.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v retrieving revision 1.561 diff -c -c -r1.561 pg_dump.c *** src/bin/pg_dump/pg_dump.c 24 Dec 2009 22:09:23 -0000 1.561 --- src/bin/pg_dump/pg_dump.c 26 Dec 2009 16:50:35 -0000 *************** *** 6542,6553 **** check_sql_result(res, g_conn, query->data, PGRES_TUPLES_OK); num = PQntuples(res); - /* should be at least 1 value */ - if (num == 0) - { - write_msg(NULL, "no label definitions found for enum ID %u\n", tyinfo->dobj.catId.oid); - exit_nicely(); - } /* * DROP must be fully qualified in case same name appears in pg_catalog. --- 6542,6547 ----
Bruce Momjian wrote: > Bruce Momjian wrote: > > Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.us> writes: > > > > Tom Lane wrote: > > > >> The reason I don't want to do it that way is that then you need two > > > >> ugly kluges in the backend, not just one. With the zero-and-add-one > > > >> approach there is no need to have a "next enum oid" variable at all. > > > > > > > Uh, I still need that variable because that is how we are going to set > > > > the oid in EnumValuesCreate(), unless we want to add dummy oid-value > > > > arguments to that function for use only by the binary upgrade > > > > server-side function. > > > > > > Please go back and re-read what I suggested: you need a function along > > > the lines of > > > add_enum_member(enum-type, 'value name', value-oid) > > > and then there's no need for any saved state. So what if it has a > > > different signature from the other pg_migrator special functions? > > > It's not doing the same thing. > > > > OK, right, I can get rid of the enum function that just sets the next > > oid value if I do all the enum value creation via function calls. I > > will work in that direction then. > > There is only one call to EnumValuesCreate() so maybe adding a > binary-upgrade-only parameter to the function will be the cleanest > approach. Here is a patch to allow EnumValuesCreate() to create labels with specified oids, with pg_dump support. This is done cleanly now that we allow zero-label enums. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/backend/catalog/pg_enum.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/catalog/pg_enum.c,v retrieving revision 1.11 diff -c -c -r1.11 pg_enum.c *** src/backend/catalog/pg_enum.c 24 Dec 2009 22:17:58 -0000 1.11 --- src/backend/catalog/pg_enum.c 26 Dec 2009 18:48:55 -0000 *************** *** 33,39 **** * vals is a list of Value strings. */ void ! EnumValuesCreate(Oid enumTypeOid, List *vals) { Relation pg_enum; TupleDesc tupDesc; --- 33,40 ---- * vals is a list of Value strings. */ void ! EnumValuesCreate(Oid enumTypeOid, List *vals, ! Oid binary_upgrade_next_pg_enum_oid) { Relation pg_enum; TupleDesc tupDesc; *************** *** 58,82 **** tupDesc = pg_enum->rd_att; /* ! * Allocate oids. While this method does not absolutely guarantee that we ! * generate no duplicate oids (since we haven't entered each oid into the ! * table before allocating the next), trouble could only occur if the oid ! * counter wraps all the way around before we finish. Which seems ! * unlikely. */ oids = (Oid *) palloc(num_elems * sizeof(Oid)); ! for (elemno = 0; elemno < num_elems; elemno++) { /* ! * The pg_enum.oid is stored in user tables. This oid must be ! * preserved by binary upgrades. */ ! oids[elemno] = GetNewOid(pg_enum); } - /* sort them, just in case counter wrapped from high to low */ - qsort(oids, num_elems, sizeof(Oid), oid_cmp); - /* and make the entries */ memset(nulls, false, sizeof(nulls)); --- 59,97 ---- tupDesc = pg_enum->rd_att; /* ! * Allocate oids */ oids = (Oid *) palloc(num_elems * sizeof(Oid)); ! if (OidIsValid(binary_upgrade_next_pg_enum_oid)) ! { ! if (num_elems != 1) ! ereport(ERROR, ! (errcode(ERRCODE_INVALID_PARAMETER_VALUE), ! errmsg("EnumValuesCreate() can only set a single OID"))); ! oids[0] = binary_upgrade_next_pg_enum_oid; ! binary_upgrade_next_pg_enum_oid = InvalidOid; ! } ! else { /* ! * While this method does not absolutely guarantee that we generate ! * no duplicate oids (since we haven't entered each oid into the ! * table before allocating the next), trouble could only occur if ! * the oid counter wraps all the way around before we finish. Which ! * seems unlikely. */ ! for (elemno = 0; elemno < num_elems; elemno++) ! { ! /* ! * The pg_enum.oid is stored in user tables. This oid must be ! * preserved by binary upgrades. ! */ ! oids[elemno] = GetNewOid(pg_enum); ! } ! /* sort them, just in case counter wrapped from high to low */ ! qsort(oids, num_elems, sizeof(Oid), oid_cmp); } /* and make the entries */ memset(nulls, false, sizeof(nulls)); Index: src/backend/commands/typecmds.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/commands/typecmds.c,v retrieving revision 1.141 diff -c -c -r1.141 typecmds.c *** src/backend/commands/typecmds.c 24 Dec 2009 22:09:23 -0000 1.141 --- src/backend/commands/typecmds.c 26 Dec 2009 18:48:56 -0000 *************** *** 1161,1167 **** false); /* Type NOT NULL */ /* Enter the enum's values into pg_enum */ ! EnumValuesCreate(enumTypeOid, stmt->vals); /* * Create the array type that goes with it. --- 1161,1167 ---- false); /* Type NOT NULL */ /* Enter the enum's values into pg_enum */ ! EnumValuesCreate(enumTypeOid, stmt->vals, InvalidOid); /* * Create the array type that goes with it. Index: src/bin/pg_dump/pg_dump.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v retrieving revision 1.562 diff -c -c -r1.562 pg_dump.c *** src/bin/pg_dump/pg_dump.c 26 Dec 2009 16:55:21 -0000 1.562 --- src/bin/pg_dump/pg_dump.c 26 Dec 2009 18:48:57 -0000 *************** *** 6528,6539 **** PGresult *res; int num, i; char *label; /* Set proper schema search path so regproc references list correctly */ selectSourceSchema(tyinfo->dobj.namespace->dobj.name); ! appendPQExpBuffer(query, "SELECT enumlabel FROM pg_catalog.pg_enum " "WHERE enumtypid = '%u'" "ORDER BY oid", tyinfo->dobj.catId.oid); --- 6528,6541 ---- PGresult *res; int num, i; + Oid enum_oid; char *label; /* Set proper schema search path so regproc references list correctly */ selectSourceSchema(tyinfo->dobj.namespace->dobj.name); ! appendPQExpBuffer(query, "SELECT oid, enumlabel " ! "FROM pg_catalog.pg_enum " "WHERE enumtypid = '%u'" "ORDER BY oid", tyinfo->dobj.catId.oid); *************** *** 6556,6573 **** if (binary_upgrade) binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid); ! appendPQExpBuffer(q, "CREATE TYPE %s AS ENUM (\n", fmtId(tyinfo->dobj.name)); ! for (i = 0; i < num; i++) { ! label = PQgetvalue(res, i, 0); ! if (i > 0) ! appendPQExpBuffer(q, ",\n"); ! appendPQExpBuffer(q, " "); ! appendStringLiteralAH(q, label, fout); } appendPQExpBuffer(q, "\n);\n"); ArchiveEntry(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, tyinfo->dobj.name, tyinfo->dobj.namespace->dobj.name, --- 6558,6601 ---- if (binary_upgrade) binary_upgrade_set_type_oids_by_type_oid(q, tyinfo->dobj.catId.oid); ! appendPQExpBuffer(q, "CREATE TYPE %s AS ENUM (", fmtId(tyinfo->dobj.name)); ! ! if (!binary_upgrade) { ! /* Labels with server-assigned oids */ ! for (i = 0; i < num; i++) ! { ! label = PQgetvalue(res, i, PQfnumber(res, "enumlabel")); ! if (i > 0) ! appendPQExpBuffer(q, ","); ! appendPQExpBuffer(q, "\n "); ! appendStringLiteralAH(q, label, fout); ! } } + appendPQExpBuffer(q, "\n);\n"); + if (binary_upgrade) + { + /* Labels with dump-assigned (preserved) oids */ + for (i = 0; i < num; i++) + { + enum_oid = atooid(PQgetvalue(res, i, PQfnumber(res, "oid"))); + label = PQgetvalue(res, i, PQfnumber(res, "enumlabel")); + + if (i == 0) + appendPQExpBuffer(q, "\n-- For binary upgrade, must preserve pg_enum oids\n"); + appendPQExpBuffer(q, + "SELECT binary_upgrade.add_pg_enum_label('%u'::pg_catalog.oid, " + "'%u'::pg_catalog.oid, ", + enum_oid, tyinfo->dobj.catId.oid); + appendStringLiteralAH(q, label, fout); + appendPQExpBuffer(q, ");\n"); + } + appendPQExpBuffer(q, "\n"); + } + ArchiveEntry(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, tyinfo->dobj.name, tyinfo->dobj.namespace->dobj.name, Index: src/include/catalog/pg_enum.h =================================================================== RCS file: /cvsroot/pgsql/src/include/catalog/pg_enum.h,v retrieving revision 1.5 diff -c -c -r1.5 pg_enum.h *** src/include/catalog/pg_enum.h 1 Jan 2009 17:23:57 -0000 1.5 --- src/include/catalog/pg_enum.h 26 Dec 2009 18:49:00 -0000 *************** *** 60,66 **** /* * prototypes for functions in pg_enum.c */ ! extern void EnumValuesCreate(Oid enumTypeOid, List *vals); extern void EnumValuesDelete(Oid enumTypeOid); #endif /* PG_ENUM_H */ --- 60,67 ---- /* * prototypes for functions in pg_enum.c */ ! extern void EnumValuesCreate(Oid enumTypeOid, List *vals, ! Oid binary_upgrade_next_pg_enum_oid); extern void EnumValuesDelete(Oid enumTypeOid); #endif /* PG_ENUM_H */
Bruce Momjian wrote: > Bruce Momjian wrote: > > Bruce Momjian wrote: > > > Tom Lane wrote: > > > > Bruce Momjian <bruce@momjian.us> writes: > > > > > Tom Lane wrote: > > > > >> The reason I don't want to do it that way is that then you need two > > > > >> ugly kluges in the backend, not just one. With the zero-and-add-one > > > > >> approach there is no need to have a "next enum oid" variable at all. > > > > > > > > > Uh, I still need that variable because that is how we are going to set > > > > > the oid in EnumValuesCreate(), unless we want to add dummy oid-value > > > > > arguments to that function for use only by the binary upgrade > > > > > server-side function. > > > > > > > > Please go back and re-read what I suggested: you need a function along > > > > the lines of > > > > add_enum_member(enum-type, 'value name', value-oid) > > > > and then there's no need for any saved state. So what if it has a > > > > different signature from the other pg_migrator special functions? > > > > It's not doing the same thing. > > > > > > OK, right, I can get rid of the enum function that just sets the next > > > oid value if I do all the enum value creation via function calls. I > > > will work in that direction then. > > > > There is only one call to EnumValuesCreate() so maybe adding a > > binary-upgrade-only parameter to the function will be the cleanest > > approach. > > Here is a patch to allow EnumValuesCreate() to create labels with > specified oids, with pg_dump support. This is done cleanly now that we > allow zero-label enums. Applied. I also bumped the catalog version so pg_migrator can detect the new backend API by looking at pg_control. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > There are several pg_migrator limitations that appeared late in the 8.4 > development cycle and were impossible to fix at that point. I would > like to fix them for Postgres 8.5: > > o a user-defined composite data type > o a user-defined array data type > o a user-defined enum data type FYI, these pg_migrator restrictions are now gone when migrating to PG 8.5, even _from_ PG 8.3. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
I'm kind of curious about the heap page conversion plan. I think we have a plan for how to do page checksums now if someonesubmits it now will we have time to do the page wok to handle page conversions? Or if not, are we better off waitingtill 8.6 to get checksums? "Bruce Momjian" <bruce@momjian.us> wrote: >Bruce Momjian wrote: >> Bruce Momjian wrote: >> > Bruce Momjian wrote: >> > > Tom Lane wrote: >> > > > Bruce Momjian <bruce@momjian.us> writes: >> > > > > Tom Lane wrote: >> > > > >> The reason I don't want to do it that way is that then you need two >> > > > >> ugly kluges in the backend, not just one. With the zero-and-add-one >> > > > >> approach there is no need to have a "next enum oid" variable at all. >> > > > >> > > > > Uh, I still need that variable because that is how we are going to set >> > > > > the oid in EnumValuesCreate(), unless we want to add dummy oid-value >> > > > > arguments to that function for use only by the binary upgrade >> > > > > server-side function. >> > > > >> > > > Please go back and re-read what I suggested: you need a function along >> > > > the lines of >> > > > add_enum_member(enum-type, 'value name', value-oid) >> > > > and then there's no need for any saved state. So what if it has a >> > > > different signature from the other pg_migrator special functions? >> > > > It's not doing the same thing. >> > > >> > > OK, right, I can get rid of the enum function that just sets the next >> > > oid value if I do all the enum value creation via function calls. I >> > > will work in that direction then. >> > >> > There is only one call to EnumValuesCreate() so maybe adding a >> > binary-upgrade-only parameter to the function will be the cleanest >> > approach. >> >> Here is a patch to allow EnumValuesCreate() to create labels with >> specified oids, with pg_dump support. This is done cleanly now that we >> allow zero-label enums. > >Applied. I also bumped the catalog version so pg_migrator can detect >the new backend API by looking at pg_control. > >-- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://enterprisedb.com > > + If your life is a hard drive, Christ can be your backup. + -- Sent from my Android phone with K-9. Please excuse my brevity.
On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce@momjian.us> wrote: > Bruce Momjian wrote: >> There are several pg_migrator limitations that appeared late in the 8.4 >> development cycle and were impossible to fix at that point. I would >> like to fix them for Postgres 8.5: >> >> o a user-defined composite data type >> o a user-defined array data type >> o a user-defined enum data type > > FYI, these pg_migrator restrictions are now gone when migrating to PG > 8.5, even _from_ PG 8.3. Wow, cool. That seems like a good step forward. ...Robert
Greg Stark wrote: > I'm kind of curious about the heap page conversion plan. I think > we have a plan for how to do page checksums now if someone > submits it now will we have time to do the page wok to handle > page conversions? Or if not, are we better off waiting till 8.6 > to get checksums? Well, I think the checksums are going in the item pointers, so there isn't any new storage space --- my guess is that the page version number will control how the backend stores the checksum. Basically the backend will need to read old and new page versions. I don't think this is something pg_migrator can handle cleanly. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sun, Dec 27, 2009 at 8:13 PM, Bruce Momjian <bruce@momjian.us> wrote: > Well, I think the checksums are going in the item pointers, so there > isn't any new storage space --- my guess is that the page version number > will control how the backend stores the checksum. Basically the backend > will need to read old and new page versions. I don't think this is > something pg_migrator can handle cleanly. I thought our plan was to only read old page versions and automatically rewrite them to new page versions. We'll have to add the hooks and the page rewrite code to do that, no? Is that something we're comfortable adding in the final commitfest? -- greg
Greg Stark wrote: > On Sun, Dec 27, 2009 at 8:13 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Well, I think the checksums are going in the item pointers, so there > > isn't any new storage space --- my guess is that the page version number > > will control how the backend stores the checksum. ?Basically the backend > > will need to read old and new page versions. ?I don't think this is > > something pg_migrator can handle cleanly. > > I thought our plan was to only read old page versions and > automatically rewrite them to new page versions. We'll have to add the > hooks and the page rewrite code to do that, no? Well, the idea of only reading the old version is so we didn't have to carry around a lot of type-specific information in the backend, but I am not sure if that applies to a hint bit change. > Is that something we're comfortable adding in the final commitfest? Uh, no idea. It would be nice, of course. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Sun, Dec 27, 2009 at 5:15 PM, Bruce Momjian <bruce@momjian.us> wrote: >> Is that something we're comfortable adding in the final commitfest? > > Uh, no idea. It would be nice, of course. Do we know if there's active development in progress on page CRCs? If so, when can we expect to see a working patch? With respect to adding it in the final CommitFest, I am concerned that this might be a big enough feature that it would be destabilizing, and I'm pretty disinclined to do anything that will destabilize the tree at this point in the release cycle. Part of my concern is that we just committed a really big feature (Hot Standby) that has already had a few bug reports and also has a known issue with VACUUM FULL that needs to be addressed. It seems likely there is going to be a good deal more work that has to be done there. I'm worried that adding more big features in the relatively small amount of time that remains before we are ostensibly going to beta is going to result in a very buggy beta and/or a buggy release (I am also concerned about Streaming Replication in this regard). On the other hand, since we have yet to see an actual patch to implement page CRCs, it may be premature to draw conclusions about how destabilizing and/or invasive it will be. If it's not that bad, maybe it's OK. Another option, if it turns out that we have several major patches that we don't feel comfortable committing for 8.5, is to branch the tree prior to the release - for example, at the start of beta. Then we could commit those features for 8.6 and just apply any remaining changes for 8.5 to both branches. This is a little more work for the committers, but it has the advantage of getting the big patches into our tree early, versus leaving them elsewhere where they may bitrot or fall through the cracks, so I think it might be worth it. ...Robert
On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce@momjian.us> wrote: >> Bruce Momjian wrote: >>> There are several pg_migrator limitations that appeared late in the 8.4 >>> development cycle and were impossible to fix at that point. I would >>> like to fix them for Postgres 8.5: >>> >>> o a user-defined composite data type >>> o a user-defined array data type >>> o a user-defined enum data type >> >> FYI, these pg_migrator restrictions are now gone when migrating to PG >> 8.5, even _from_ PG 8.3. > > Wow, cool. That seems like a good step forward. It appears that the pg_migrator README needs a bit of revision to make it more clear which limitations apply to migration between which versions. In particular, the current wording suggests that NONE of the limitations apply to 8.3 -> 8.5 migrations, which is not the case - e.g. we haven't done anything about the need to rebuild certain types of indices. ...Robert
Robert Haas wrote: > On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> Bruce Momjian wrote: > >>> There are several pg_migrator limitations that appeared late in the 8.4 > >>> development cycle and were impossible to fix at that point. ?I would > >>> like to fix them for Postgres 8.5: > >>> > >>> ? ? ? ? o ?a user-defined composite data type > >>> ? ? ? ? o ?a user-defined array data type > >>> ? ? ? ? o ?a user-defined enum data type > >> > >> FYI, these pg_migrator restrictions are now gone when migrating to PG > >> 8.5, even _from_ PG 8.3. > > > > Wow, cool. ?That seems like a good step forward. > > It appears that the pg_migrator README needs a bit of revision to make > it more clear which limitations apply to migration between which > versions. In particular, the current wording suggests that NONE of > the limitations apply to 8.3 -> 8.5 migrations, which is not the case > - e.g. we haven't done anything about the need to rebuild certain > types of indices. Very true. I have just made a new pg_migrator release with an updated README file. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Mon, Dec 28, 2009 at 10:48 AM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce@momjian.us> wrote: >> >> Bruce Momjian wrote: >> >>> There are several pg_migrator limitations that appeared late in the 8.4 >> >>> development cycle and were impossible to fix at that point. ?I would >> >>> like to fix them for Postgres 8.5: >> >>> >> >>> ? ? ? ? o ?a user-defined composite data type >> >>> ? ? ? ? o ?a user-defined array data type >> >>> ? ? ? ? o ?a user-defined enum data type >> >> >> >> FYI, these pg_migrator restrictions are now gone when migrating to PG >> >> 8.5, even _from_ PG 8.3. >> > >> > Wow, cool. ?That seems like a good step forward. >> >> It appears that the pg_migrator README needs a bit of revision to make >> it more clear which limitations apply to migration between which >> versions. In particular, the current wording suggests that NONE of >> the limitations apply to 8.3 -> 8.5 migrations, which is not the case >> - e.g. we haven't done anything about the need to rebuild certain >> types of indices. > > Very true. I have just made a new pg_migrator release with an updated > README file. Ah, cool. So this seems to imply that a migration from 8.4 to 8.5 should be clear sailing. Is that correct? ...Robert
Robert Haas wrote: > On Mon, Dec 28, 2009 at 10:48 AM, Bruce Momjian <bruce@momjian.us> wrote: > > Robert Haas wrote: > >> On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > >> > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> >> Bruce Momjian wrote: > >> >>> There are several pg_migrator limitations that appeared late in the 8.4 > >> >>> development cycle and were impossible to fix at that point. ?I would > >> >>> like to fix them for Postgres 8.5: > >> >>> > >> >>> ? ? ? ? o ?a user-defined composite data type > >> >>> ? ? ? ? o ?a user-defined array data type > >> >>> ? ? ? ? o ?a user-defined enum data type > >> >> > >> >> FYI, these pg_migrator restrictions are now gone when migrating to PG > >> >> 8.5, even _from_ PG 8.3. > >> > > >> > Wow, cool. ?That seems like a good step forward. > >> > >> It appears that the pg_migrator README needs a bit of revision to make > >> it more clear which limitations apply to migration between which > >> versions. ?In particular, the current wording suggests that NONE of > >> the limitations apply to 8.3 -> 8.5 migrations, which is not the case > >> - e.g. we haven't done anything about the need to rebuild certain > >> types of indices. > > > > Very true. I have just made a new pg_migrator release with an updated > > README file. > > Ah, cool. So this seems to imply that a migration from 8.4 to 8.5 > should be clear sailing. Is that correct? Yes, so far. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +