--single-transaction hack to pg_upgrade does not work

Some of the buildfarm members are failing the pg_upgrade regression test
since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
it here, and the symptom is:

pg_restore: creating TYPE float8range
pg_restore: creating TYPE insenum
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE insenum tgl
pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);

I have not investigated why it apparently passes some places; this looks
to me like a guaranteed failure.

regards, tom lane

On 11/30/2012 11:10 PM, Tom Lane wrote:
> Some of the buildfarm members are failing the pg_upgrade regression test
> since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
> it here, and the symptom is:
>
> pg_restore: creating TYPE float8range
> pg_restore: creating TYPE insenum
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE insenum tgl
> pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
> Command was:
> -- For binary upgrade, must preserve pg_type oid
> SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
>
> I have not investigated why it apparently passes some places; this looks
> to me like a guaranteed failure.
>
>

Testing pg_upgrade has only been in buildfarm releases since September
28, and even then is optional, although enabled by default in the sample
config file. Looks like even I need to upgrade a few of my animals to do
it. It probably needs to improve its error logging though.

Seems odd not to have run "make check" before committing, though.

cheers

andrew

On Sat, Dec 1, 2012 at 07:43:17AM -0500, Andrew Dunstan wrote:
>
> On 11/30/2012 11:10 PM, Tom Lane wrote:
> >Some of the buildfarm members are failing the pg_upgrade regression test
> >since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
> >it here, and the symptom is:
> >
> >pg_restore: creating TYPE float8range
> >pg_restore: creating TYPE insenum
> >pg_restore: [archiver (db)] Error while PROCESSING TOC:
> >pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE insenum tgl
> >pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
> > Command was:
> >-- For binary upgrade, must preserve pg_type oid
> >SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
> >
> >I have not investigated why it apparently passes some places; this looks
> >to me like a guaranteed failure.

I see now. Sorry. I was so focused on performance testing and never
thought this cause pg_upgrade to fail. I did not run my full tests this
time.

It seems the problem is that we bundling the pg_upgrade oid set function
into the same code block as ALTER TYPE, to preserve the type oid. Let
me see how to fix this.

Should I do something temporarily to get the buildfarm green again?
Just revert the entire thing?

> Testing pg_upgrade has only been in buildfarm releases since
> September 28, and even then is optional, although enabled by default
> in the sample config file. Looks like even I need to upgrade a few
> of my animals to do it. It probably needs to improve its error
> logging though.
>
> Seems odd not to have run "make check" before committing, though.

I was not aware the pg_upgrade testing was in our git tree; I thought
it was only in the buildfarm code. I am glad it is in our tree and it
seem to do my full tests in a more automated manner. I will use it in
the future.

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

+ It's impossible for everything to be true. +

On Sat, Dec 1, 2012 at 10:25:10AM -0500, Bruce Momjian wrote:
> On Sat, Dec 1, 2012 at 07:43:17AM -0500, Andrew Dunstan wrote:
> >
> > On 11/30/2012 11:10 PM, Tom Lane wrote:
> > >Some of the buildfarm members are failing the pg_upgrade regression test
> > >since commit 12ee6ec71f8754ff3573711032b9b4d5a764ba84. I can duplicate
> > >it here, and the symptom is:
> > >
> > >pg_restore: creating TYPE float8range
> > >pg_restore: creating TYPE insenum
> > >pg_restore: [archiver (db)] Error while PROCESSING TOC:
> > >pg_restore: [archiver (db)] Error from TOC entry 978; 1247 16584 TYPE insenum tgl
> > >pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
> > > Command was:
> > >-- For binary upgrade, must preserve pg_type oid
> > >SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
> > >
> > >I have not investigated why it apparently passes some places; this looks
> > >to me like a guaranteed failure.
>
> I see now. Sorry. I was so focused on performance testing and never
> thought this cause pg_upgrade to fail. I did not run my full tests this
> time.
>
> It seems the problem is that we bundling the pg_upgrade oid set function
> into the same code block as ALTER TYPE, to preserve the type oid. Let
> me see how to fix this.
>
> Should I do something temporarily to get the buildfarm green again?
> Just revert the entire thing?

OK, I found the problem, and it isn't good. Our manual clearly says:

ALTER TYPE ... ADD VALUE (the form that adds a new value
to an enum type) cannot be executed inside a transaction block.

This also means it can't be passed inside an implicit transaction block,
which happens when you pass:

SELECT 1; SELECT 2;

as a string, and I think this is what pg_restore is doing. So, not only
is --single-transction causing the failure, but even without
--single-transction, pg_restore just passes the multi-statement string
to the backend, and you get the error:

pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE
... ADD cannot run inside a transaction block
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);

psql dutifully splits up the string into separate commands, which is why
the previous pg_dumpall | psql coding worked. One simple fix would be
to revert to plain output format, and return to using psql. Of course,
we lose a lot of performance with that. The pending AtOEXAct patch gets
us most of the performance back:

#tbls git -1 AtOEXAct both
1 11.06 13.06 10.99 13.20
1000 21.71 22.92 22.20 22.51
2000 32.86 31.09 32.51 31.62
4000 55.22 49.96 52.50 49.99
8000 105.34 82.10 95.32 82.94
16000 223.67 164.27 187.40 159.53
32000 543.93 324.63 366.44 317.93
64000 1697.14 791.82 767.32 752.57

so maybe that's how we have to go, or modify pg_dump to emit the
binary-upgrade function call as a separate pg_dump entry, rather than
lumping it in with ALTER TYPE ... ADD VALUE.

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

+ It's impossible for everything to be true. +

On Sat, Dec 1, 2012 at 10:41:06AM -0500, Bruce Momjian wrote:
> OK, I found the problem, and it isn't good. Our manual clearly says:
>
> ALTER TYPE ... ADD VALUE (the form that adds a new value
> to an enum type) cannot be executed inside a transaction block.
>
> This also means it can't be passed inside an implicit transaction block,
> which happens when you pass:
>
> SELECT 1; SELECT 2;
>
> as a string, and I think this is what pg_restore is doing. So, not only
> is --single-transction causing the failure, but even without
> --single-transction, pg_restore just passes the multi-statement string
> to the backend, and you get the error:
>
> pg_restore: [archiver (db)] could not execute query: ERROR: ALTER TYPE
> ... ADD cannot run inside a transaction block
> Command was:
> -- For binary upgrade, must preserve pg_type oid
> SELECT binary_upgrade.set_next_pg_type_oid('16584'::pg_catalog.oid);
>
> psql dutifully splits up the string into separate commands, which is why
> the previous pg_dumpall | psql coding worked. One simple fix would be
> to revert to plain output format, and return to using psql. Of course,
> we lose a lot of performance with that. The pending AtOEXAct patch gets
> us most of the performance back:
>
> #tbls git -1 AtOEXAct both
> 1 11.06 13.06 10.99 13.20
> 1000 21.71 22.92 22.20 22.51
> 2000 32.86 31.09 32.51 31.62
> 4000 55.22 49.96 52.50 49.99
> 8000 105.34 82.10 95.32 82.94
> 16000 223.67 164.27 187.40 159.53
> 32000 543.93 324.63 366.44 317.93
> 64000 1697.14 791.82 767.32 752.57
>
> so maybe that's how we have to go, or modify pg_dump to emit the
> binary-upgrade function call as a separate pg_dump entry, rather than
> lumping it in with ALTER TYPE ... ADD VALUE.

Scratch that idea. By definition, no matter how we modify pg_dump or
pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run
in a multi-statement transaction, so we have to certainly remove
--single-transction, and then we can decide if we want to continue using
pg_restore with an improved pg_dump, or just fall back to pg_dump and
psql.

I am thinking at this point I should just switch to pg_dump text format
and psql to get the build farm green again, but not lose the other
changes that give us per-database dumps.

This does make me wonder why pg_restore supports --single-transaction if
it has known failure cases (that are not documented in the pg_restore
manual page, only in the ALTER TYPE manual page). Are users really
going to know if their database has objects that are not supported by
--single-transaction?

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

+ It's impossible for everything to be true. +

On 2012-12-01 10:55:09 -0500, Bruce Momjian wrote:
> On Sat, Dec 1, 2012 at 10:41:06AM -0500, Bruce Momjian wrote:
> > OK, I found the problem, and it isn't good. Our manual clearly says:
> >
> > ALTER TYPE ... ADD VALUE (the form that adds a new value
> > to an enum type) cannot be executed inside a transaction block.
> >
> > so maybe that's how we have to go, or modify pg_dump to emit the
> > binary-upgrade function call as a separate pg_dump entry, rather than
> > lumping it in with ALTER TYPE ... ADD VALUE.
>
> Scratch that idea. By definition, no matter how we modify pg_dump or
> pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run
> in a multi-statement transaction, so we have to certainly remove
> --single-transction, and then we can decide if we want to continue using
> pg_restore with an improved pg_dump, or just fall back to pg_dump and
> psql.
>
> I am thinking at this point I should just switch to pg_dump text format
> and psql to get the build farm green again, but not lose the other
> changes that give us per-database dumps.
>
> This does make me wonder why pg_restore supports --single-transaction if
> it has known failure cases (that are not documented in the pg_restore
> manual page, only in the ALTER TYPE manual page). Are users really
> going to know if their database has objects that are not supported by
> --single-transaction?

Could we possibly allow adding enum values to a type which was just created in
this transaction? That shouldn't be too hard. At least easier than providing
the capability to pre-assign the next N oids...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-01 10:55:09 -0500, Bruce Momjian wrote:
> This does make me wonder why pg_restore supports --single-transaction if
> it has known failure cases (that are not documented in the pg_restore
> manual page, only in the ALTER TYPE manual page). Are users really
> going to know if their database has objects that are not supported by
> --single-transaction?

That problem only exists in binary upgrade mode, in plain mode the enum
is created with all values in one CREATE TYPE ... AS ENUM(...)
statement. So the problem simply doesn't exist there.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> This does make me wonder why pg_restore supports --single-transaction if
> it has known failure cases (that are not documented in the pg_restore
> manual page, only in the ALTER TYPE manual page).

AFAIR, the ADD VALUE path is only taken with --binary-upgrade, which
is just about entirely undocumented anyway.

regards, tom lane

On Sat, Dec 1, 2012 at 10:55:09AM -0500, Bruce Momjian wrote:
> Scratch that idea. By definition, no matter how we modify pg_dump or
> pg_restore, ALTER TYPE ... ADD VALUE is never going to be able to be run
> in a multi-statement transaction, so we have to certainly remove
> --single-transction, and then we can decide if we want to continue using
> pg_restore with an improved pg_dump, or just fall back to pg_dump and
> psql.
>
> I am thinking at this point I should just switch to pg_dump text format
> and psql to get the build farm green again, but not lose the other
> changes that give us per-database dumps.
>
> This does make me wonder why pg_restore supports --single-transaction if
> it has known failure cases (that are not documented in the pg_restore
> manual page, only in the ALTER TYPE manual page). Are users really
> going to know if their database has objects that are not supported by
> --single-transaction?

OK, Andrew has accurately told me via IM that ALTER TYPE ... ADD VALUE
is only emitted by pg_dump in binary-upgrade mode. Seems you can run it
manually, but pg_dump doesn't use it except for binary-upgrade mode, and
I now see that in the code.

So, that removes my concern about pg_restore --single-transaction in
general.

So, we have to decide if we should improve pg_dump to split up the
function call and ALTER TYPE ... ADD VALUE command, or fall back to text
dump mode and psql. That removes the optimization of using custom
format, and the optimization of using pg_restore. However, I don't see
how I can guarantee that the pg_upgrade oid setting function will be
called just _before_ the ALTER TYPE ... ADD VALUE command without having
them in the same command string package.

Shame --- pg_upgrade performance was improving so steadily, I was hoping
to see negative duration times soon. ;-)

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

+ It's impossible for everything to be true. +

On Sat, Dec 1, 2012 at 11:11:31AM -0500, Bruce Momjian wrote:
> Shame --- pg_upgrade performance was improving so steadily, I was hoping
> to see negative duration times soon. ;-)

Is that the definition of optimism? :-)

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

+ It's impossible for everything to be true. +

On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
> Could we possibly allow adding enum values to a type which was just created in
> this transaction? That shouldn't be too hard. At least easier than providing
> the capability to pre-assign the next N oids...

The attached patch does just that. Its *not* ready yet though, as it
will be apparent for everyone who reads it ;)

To really make that work in a reliable manner we would probably need
an rd_createSubid for typcache entries instead of testing xmin as I have
done here?

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-01 17:36:20 +0100, Andres Freund wrote:
> On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
> > Could we possibly allow adding enum values to a type which was just created in
> > this transaction? That shouldn't be too hard. At least easier than providing
> > the capability to pre-assign the next N oids...
>
> The attached patch does just that. Its *not* ready yet though, as it
> will be apparent for everyone who reads it ;)
>
> To really make that work in a reliable manner we would probably need
> an rd_createSubid for typcache entries instead of testing xmin as I have
> done here?

And the patch...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> The attached patch does just that. Its *not* ready yet though, as it
>> will be apparent for everyone who reads it ;)

ISTM this sort of thing ought to be safe enough, though you probably
need to insist both that the pg_type row's xmin be current XID and
that it not be HEAP_UPDATED.

>> To really make that work in a reliable manner we would probably need
>> an rd_createSubid for typcache entries instead of testing xmin as I have
>> done here?

What's more reliable about that? For one thing, cache entries can get
flushed. The relcache goes to some lengths to hang onto rd_createSubid
anyway, but I don't want to put equivalent logic into typcache.

regards, tom lane

On 12/01/2012 11:38 AM, Andres Freund wrote:
> On 2012-12-01 17:36:20 +0100, Andres Freund wrote:
>> On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
>>> Could we possibly allow adding enum values to a type which was just created in
>>> this transaction? That shouldn't be too hard. At least easier than providing
>>> the capability to pre-assign the next N oids...
>> The attached patch does just that. Its *not* ready yet though, as it
>> will be apparent for everyone who reads it ;)
>>
>> To really make that work in a reliable manner we would probably need
>> an rd_createSubid for typcache entries instead of testing xmin as I have
>> done here?

Does this actually get you over the problem identified in the comment?:

* We disallow this in transaction blocks, because we can't cope
* with enum OID values getting into indexes and then having their
* defining pg_enum entries go away.

cheers

andrew

On Sat, Dec 1, 2012 at 05:36:20PM +0100, Andres Freund wrote:
> On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
> > Could we possibly allow adding enum values to a type which was just created in
> > this transaction? That shouldn't be too hard. At least easier than providing
> > the capability to pre-assign the next N oids...
>
> The attached patch does just that. Its *not* ready yet though, as it
> will be apparent for everyone who reads it ;)
>
> To really make that work in a reliable manner we would probably need
> an rd_createSubid for typcache entries instead of testing xmin as I have
> done here?

I can confirm that this patch allows pg_upgrade's test.sh to pass. :-)

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

+ It's impossible for everything to be true. +

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Does this actually get you over the problem identified in the comment?:

> * We disallow this in transaction blocks, because we can't cope
> * with enum OID values getting into indexes and then having their
> * defining pg_enum entries go away.

Why wouldn't it? If the enum type was created in the current xact, then
surely any table columns of the type, or a fortiori indexes on the type,
were also created in the current xact and they'd all go away on abort.

regards, tom lane

On 12/01/2012 12:06 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Does this actually get you over the problem identified in the comment?:
>> * We disallow this in transaction blocks, because we can't cope
>> * with enum OID values getting into indexes and then having their
>> * defining pg_enum entries go away.
> Why wouldn't it? If the enum type was created in the current xact, then
> surely any table columns of the type, or a fortiori indexes on the type,
> were also created in the current xact and they'd all go away on abort.
>
>

OK, I understand. So this seems like a Good Thing to do.

cheers

andrew

On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >> The attached patch does just that. Its *not* ready yet though, as it
> >> will be apparent for everyone who reads it ;)
>
> ISTM this sort of thing ought to be safe enough, though you probably
> need to insist both that the pg_type row's xmin be current XID and
> that it not be HEAP_UPDATED.
>
> >> To really make that work in a reliable manner we would probably need
> >> an rd_createSubid for typcache entries instead of testing xmin as I have
> >> done here?
>
> What's more reliable about that? For one thing, cache entries can get
> flushed. The relcache goes to some lengths to hang onto rd_createSubid
> anyway, but I don't want to put equivalent logic into typcache.

I was concerned about updated rows but forgot about HEAP_UPDATED. So I
thought that it would be possible to alter the type in some generic
fashion (e.g. change owner) and then add new values.

The typecache variant would also have some hope of allowing some
intermediate changes to the type (like changing the type as above) in
the same transaction while still allowing to add new values.

But then, all that is not necessary for pg_upgrade.

Let me provide something a littlebit more mature.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-01 12:01:17 -0500, Andrew Dunstan wrote:
>
> On 12/01/2012 11:38 AM, Andres Freund wrote:
> >On 2012-12-01 17:36:20 +0100, Andres Freund wrote:
> >>On 2012-12-01 17:03:03 +0100, Andres Freund wrote:
> >>>Could we possibly allow adding enum values to a type which was just created in
> >>>this transaction? That shouldn't be too hard. At least easier than providing
> >>>the capability to pre-assign the next N oids...
> >>The attached patch does just that. Its *not* ready yet though, as it
> >>will be apparent for everyone who reads it ;)
> >>
> >>To really make that work in a reliable manner we would probably need
> >>an rd_createSubid for typcache entries instead of testing xmin as I have
> >>done here?
>
>
> Does this actually get you over the problem identified in the comment?:
>
> * We disallow this in transaction blocks, because we can't cope
> * with enum OID values getting into indexes and then having their
> * defining pg_enum entries go away.

I don't see why not at least. No index that can contain values from the enum
will survive a transaction abort or can be seen from the outside before it
committed.

So I don't see a problem. What made you concerned?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
>> ISTM this sort of thing ought to be safe enough, though you probably
>> need to insist both that the pg_type row's xmin be current XID and
>> that it not be HEAP_UPDATED.

> I was concerned about updated rows but forgot about HEAP_UPDATED. So I
> thought that it would be possible to alter the type in some generic
> fashion (e.g. change owner) and then add new values.

Yeah, I was just thinking about that: we'd have to fail if pg_dump
emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more
values. Fortunately it doesn't do that; the ADD VALUE business is
just a multi-statement expansion of CREATE TYPE AS ENUM, and any
other ALTERs will come afterwards.

> Let me provide something a littlebit more mature.

It could do with some comments ;-)

regards, tom lane

On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
> >> ISTM this sort of thing ought to be safe enough, though you probably
> >> need to insist both that the pg_type row's xmin be current XID and
> >> that it not be HEAP_UPDATED.
>
> > I was concerned about updated rows but forgot about HEAP_UPDATED. So I
> > thought that it would be possible to alter the type in some generic
> > fashion (e.g. change owner) and then add new values.
>
> Yeah, I was just thinking about that: we'd have to fail if pg_dump
> emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more
> values. Fortunately it doesn't do that; the ADD VALUE business is
> just a multi-statement expansion of CREATE TYPE AS ENUM, and any
> other ALTERs will come afterwards.

Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats
luckily just fine.

> > Let me provide something a littlebit more mature.
>
> It could do with some comments ;-)

Hehe, yes. Hopefully this version has enough of that.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Dec 1, 2012 at 07:32:48PM +0100, Andres Freund wrote:
> On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
> > >> ISTM this sort of thing ought to be safe enough, though you probably
> > >> need to insist both that the pg_type row's xmin be current XID and
> > >> that it not be HEAP_UPDATED.
> >
> > > I was concerned about updated rows but forgot about HEAP_UPDATED. So I
> > > thought that it would be possible to alter the type in some generic
> > > fashion (e.g. change owner) and then add new values.
> >
> > Yeah, I was just thinking about that: we'd have to fail if pg_dump
> > emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more
> > values. Fortunately it doesn't do that; the ADD VALUE business is
> > just a multi-statement expansion of CREATE TYPE AS ENUM, and any
> > other ALTERs will come afterwards.
>
> Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats
> luckily just fine.

Do we need a comment in pg_dump.c to make sure that doesn't change?

> > > Let me provide something a littlebit more mature.
> >
> > It could do with some comments ;-)
>
> Hehe, yes. Hopefully this version has enough of that.

I believe this text in alter_type.sgml need updating:

<command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
enum type) cannot be executed inside a transaction block.

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

+ It's impossible for everything to be true. +

On 2012-12-01 13:43:44 -0500, Bruce Momjian wrote:
> On Sat, Dec 1, 2012 at 07:32:48PM +0100, Andres Freund wrote:
> > On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> > > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > > On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
> > > >> ISTM this sort of thing ought to be safe enough, though you probably
> > > >> need to insist both that the pg_type row's xmin be current XID and
> > > >> that it not be HEAP_UPDATED.
> > >
> > > > I was concerned about updated rows but forgot about HEAP_UPDATED. So I
> > > > thought that it would be possible to alter the type in some generic
> > > > fashion (e.g. change owner) and then add new values.
> > >
> > > Yeah, I was just thinking about that: we'd have to fail if pg_dump
> > > emitted CREATE TYPE, ALTER TYPE OWNER, and then tried to add more
> > > values. Fortunately it doesn't do that; the ADD VALUE business is
> > > just a multi-statement expansion of CREATE TYPE AS ENUM, and any
> > > other ALTERs will come afterwards.
> >
> > Well, there's a binary_upgrade.set_next_pg_enum_oid() inbetween, but thats
> > luckily just fine.
>
> Do we need a comment in pg_dump.c to make sure that doesn't change?

We could, but I don't really see it likely that somethig problematic
will be added there the regression tests should catch any problem
there (right?).

> > > > Let me provide something a littlebit more mature.
> > >
> > > It could do with some comments ;-)
> >
> > Hehe, yes. Hopefully this version has enough of that.
>
> I believe this text in alter_type.sgml need updating:
>
> <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
> enum type) cannot be executed inside a transaction block.

I purposefully didn't change that because the new support is rather
minimalistic. E.g. BEGIN; CREATE TYPE foo AS ENUM(); ALTER TYPE foo
RENAME TO bar; ALTER TYPE bar ADD VALUE 'blub'; COMMIT; is not going to
work. So it seems best not to make it something official but keep it as
an extension for pg_upgrade support.

(btw, the commit message inside the git am'able patch contained
that explanation...)

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-12-01 13:43:44 -0500, Bruce Momjian wrote:
>> I believe this text in alter_type.sgml need updating:
>>
>> <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
>> enum type) cannot be executed inside a transaction block.

> I purposefully didn't change that because the new support is rather
> minimalistic.

Yeah, I tend to agree. There are a lot of cases that people might think
should work that won't, and anyway it's not clear what the use-case is
for this beyond pg_dump's very specific usage.

regards, tom lane

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
>> It could do with some comments ;-)

> Hehe, yes. Hopefully this version has enough of that.

Hm, maybe too many --- I don't really think it's necessary for utility.c
to provide a redundant explanation of what's happening.

Committed with adjustments --- mainly, the
TransactionIdIsCurrentTransactionId test was flat out wrong, because it
would accept a parent transaction ID as well as a subcommitted
subtransaction ID. We could safely allow the latter, but I don't think
it's worth the trouble to add another xact.c test function.

regards, tom lane

On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> >> It could do with some comments ;-)
>
> > Hehe, yes. Hopefully this version has enough of that.
>
> Hm, maybe too many --- I don't really think it's necessary for utility.c
> to provide a redundant explanation of what's happening.
>
> Committed with adjustments --- mainly, the
> TransactionIdIsCurrentTransactionId test was flat out wrong, because it
> would accept a parent transaction ID as well as a subcommitted
> subtransaction ID. We could safely allow the latter, but I don't think
> it's worth the trouble to add another xact.c test function.

Thanks everyone. I can confirm that pg_upgrades make "check now"
passes, so this should green the buildfarm. Again, I aplogize for the
fire drill.

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

+ It's impossible for everything to be true. +

On 2012-12-01 14:31:03 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> >> It could do with some comments ;-)
>
> > Hehe, yes. Hopefully this version has enough of that.
>
> Hm, maybe too many --- I don't really think it's necessary for utility.c
> to provide a redundant explanation of what's happening.

Yea, was in doubt about that. Added it because it felt a bit strange
to pass down isTopLevel.

> Committed with adjustments --- mainly, the
> TransactionIdIsCurrentTransactionId test was flat out wrong, because it
> would accept a parent transaction ID as well as a subcommitted
> subtransaction ID. We could safely allow the latter, but I don't think
> it's worth the trouble to add another xact.c test function.

Yea, I plainly oversaw that it would be 'dangerous' for a toplevel txn
if a subtransaction aborts. I don't really see a usecase for supporting
subtxns either, so the current GetCurrentTransactionId() seems sensible.

Thanks.

Andres

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/01/2012 02:34 PM, Bruce Momjian wrote:
> On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>> On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
>>>> It could do with some comments ;-)
>>> Hehe, yes. Hopefully this version has enough of that.
>> Hm, maybe too many --- I don't really think it's necessary for utility.c
>> to provide a redundant explanation of what's happening.
>>
>> Committed with adjustments --- mainly, the
>> TransactionIdIsCurrentTransactionId test was flat out wrong, because it
>> would accept a parent transaction ID as well as a subcommitted
>> subtransaction ID. We could safely allow the latter, but I don't think
>> it's worth the trouble to add another xact.c test function.
> Thanks everyone. I can confirm that pg_upgrades make "check now"
> passes, so this should green the buildfarm. Again, I aplogize for the
> fire drill.
>

I've added better logging of pg_upgrade testing to the buildfarm module:
<https://github.com/PGBuildFarm/client-code/commit/83834cceaea95ba42c03a1079a8c768782e32a6b>
example is at
<http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=crake&dt=2012-12-01%2017%3A44%3A03>
This will be in the next buildfarm client release.

cheers

andrew

On Sat, Dec 1, 2012 at 03:41:15PM -0500, Andrew Dunstan wrote:
>
> On 12/01/2012 02:34 PM, Bruce Momjian wrote:
> >On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote:
> >>Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>>On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
> >>>>It could do with some comments ;-)
> >>>Hehe, yes. Hopefully this version has enough of that.
> >>Hm, maybe too many --- I don't really think it's necessary for utility.c
> >>to provide a redundant explanation of what's happening.
> >>
> >>Committed with adjustments --- mainly, the
> >>TransactionIdIsCurrentTransactionId test was flat out wrong, because it
> >>would accept a parent transaction ID as well as a subcommitted
> >>subtransaction ID. We could safely allow the latter, but I don't think
> >>it's worth the trouble to add another xact.c test function.
> >Thanks everyone. I can confirm that pg_upgrades make "check now"
> >passes, so this should green the buildfarm. Again, I aplogize for the
> >fire drill.
> >
>
>
> I've added better logging of pg_upgrade testing to the buildfarm
> module: <https://github.com/PGBuildFarm/client-code/commit/83834cceaea95ba42c03a1079a8c768782e32a6b>
> example is at <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=crake&dt=2012-12-01%2017%3A44%3A03>
> This will be in the next buildfarm client release.

Wow, that looks great. You even show the last few lines from the log
files!

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

+ It's impossible for everything to be true. +