Re: 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)dunslane(dot)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 dangerous about
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(at)dunslane(dot)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 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?

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(at)commandprompt(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

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(at)momjian(dot)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(at)commandprompt(dot)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(at)momjian(dot)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(at)momjian(dot)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;
}

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(at)joeconway(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Sun, Dec 20, 2009 at 1:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sun, Dec 20, 2009 at 1:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)momjian(dot)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(at)hub(dot)org http://www.hub.org

Yahoo:yscrappy Skype: hub.org ICQ:7615664 MSN:scrappy(at)hub(dot)org

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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 with the sequence?

A sequences would presumably eliminate all these issues. Even if we wanted to disallow user access to the sequence, having something that's not competing with all the other uses of OID would presumably make this a lot simpler.
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

decibel <decibel(at)decibel(dot)org> writes:
> Is there some reason why OIDs were used for ENUM instead of a general sequence? Were we worried about people screwing with the 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, having something 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(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)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(at)mit(dot)edu> writes:
> On Wed, Dec 23, 2009 at 7:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)mit(dot)edu> writes:
> On Wed, Dec 23, 2009 at 8:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)mit(dot)edu> writes:
> > On Wed, Dec 23, 2009 at 7:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 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.:

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> I 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> I 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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 oids
SELECT 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> I 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> > 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Bruce Momjian wrote:
> > > Tom Lane wrote:
> > > > Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:
> 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(at)momjian(dot)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 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?

"Bruce Momjian" <bruce(at)momjian(dot)us> wrote:

>Bruce Momjian wrote:
>> Bruce Momjian wrote:
>> > Bruce Momjian wrote:
>> > > Tom Lane wrote:
>> > > > Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)gmail(dot)com> wrote:
> On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> wrote:
> > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> On Sun, Dec 27, 2009 at 2:16 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> > On Sun, Dec 27, 2009 at 9:53 AM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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