--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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.57so 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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Bruce Momjian <bruce@momjian.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
0001-Allow-ALTER-TYPE-.-ADD-VALUE-inside-transactions-if-.patchtext/x-patch; charset=us-asciiDownload
>From 2839c7037d4ca8903a322aba5c399f2e54f2d63b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 1 Dec 2012 17:37:57 +0100
Subject: [PATCH] Allow ALTER TYPE ... ADD VALUE inside transactions if the
type was created in the same transaction
---
src/backend/commands/typecmds.c | 16 ++++++++++++++--
src/backend/tcop/utility.c | 4 ++--
src/include/commands/typecmds.h | 2 +-
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8418096..d419ed0 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1169,11 +1169,14 @@ DefineEnum(CreateEnumStmt *stmt)
* Adds a new label to an existing enum.
*/
void
-AlterEnum(AlterEnumStmt *stmt)
+AlterEnum(AlterEnumStmt *stmt, bool toplevel)
{
Oid enum_type_oid;
TypeName *typename;
HeapTuple tup;
+ bool in_transaction;
+
+ in_transaction = IsTransactionBlock() || IsSubTransaction() || !toplevel;
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(stmt->typeName);
@@ -1183,12 +1186,21 @@ AlterEnum(AlterEnumStmt *stmt)
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
+ if (in_transaction)
+ {
+ TransactionId xmin = HeapTupleHeaderGetXmin(tup->t_data);
+ if (!TransactionIdIsCurrentTransactionId(xmin))
+ PreventTransactionChain(toplevel, "ALTER TYPE ... ADD2");
+ }
+ else
+ PreventTransactionChain(toplevel, "ALTER TYPE ... ADD");
+
/* Check it's an enum and check user has permission to ALTER the enum */
checkEnumOwner(tup);
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
- stmt->newValNeighbor, stmt->newValIsAfter,
+ stmt->newValNeighbor, stmt->newValIsAfter,
stmt->skipIfExists);
ReleaseSysCache(tup);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 491bd29..bf2a0e3 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -977,9 +977,9 @@ standard_ProcessUtility(Node *parsetree,
* 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.
+ * XXX
*/
- PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD");
- AlterEnum((AlterEnumStmt *) parsetree);
+ AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
break;
case T_ViewStmt: /* CREATE VIEW */
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index 2351024..792b146 100644
--- a/src/include/commands/typecmds.h
+++ b/src/include/commands/typecmds.h
@@ -26,7 +26,7 @@ extern void RemoveTypeById(Oid typeOid);
extern void DefineDomain(CreateDomainStmt *stmt);
extern void DefineEnum(CreateEnumStmt *stmt);
extern void DefineRange(CreateRangeStmt *stmt);
-extern void AlterEnum(AlterEnumStmt *stmt);
+extern void AlterEnum(AlterEnumStmt *stmt, bool toplevel);
extern Oid DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
--
1.7.10.4
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew@dunslane.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/01/2012 12:06 PM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.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
Attachments:
0001-Allow-adding-new-labels-to-enums-inside-a-transactio.patchtext/x-patch; charset=us-asciiDownload
>From 7288d2cfdd7300bc665ecbfa43640814e665dad1 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 1 Dec 2012 17:37:57 +0100
Subject: [PATCH] Allow adding new labels to enums inside a transaction if
enum was created in the same txn
Normally it is not safe to do so because the enum values could appear in
indexes even though the transaction aborted but if the enum was originally
created in the same transaction thats not a problem because all indexes
containing the new label won't survive that anyway.
The check employed for testing whether the enum was created in the same txn can
miss some valid cases but it should never miss a case where it would be invalid
to allow this case.
The reason to allow this somewhat strange looking, after all why alter an enum
created in the same txn, case is that pg_dump --binary-upgrade emits CREATE
TYPE typename AS ENUM(); separately from ALTER TYPE ... ADD VALUE to be able to
set the oids of the individual enum labels. Being able to employ
--single-transaction mode during restore speeds up pg_upgrade.
Don't document the relaxation of this restriction in user visible
documentation, it has a too limited scope to be generally interesting.
---
src/backend/access/heap/rewriteheap.c | 2 +-
src/backend/commands/typecmds.c | 36 +++++++++++++++++++++++++++++++--
src/backend/tcop/utility.c | 14 ++++++++-----
src/include/access/htup_details.h | 5 +++++
src/include/commands/typecmds.h | 2 +-
src/test/regress/expected/enum.out | 24 ++++++++++++++++++++++
src/test/regress/sql/enum.sql | 28 +++++++++++++++++++++++++
7 files changed, 102 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 0f67a80..ae42b2d 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -426,7 +426,7 @@ rewrite_heap_tuple(RewriteState state,
* previous tuple's xmax would equal this one's xmin, so it's
* RECENTLY_DEAD if and only if the xmin is not before OldestXmin.
*/
- if ((new_tuple->t_data->t_infomask & HEAP_UPDATED) &&
+ if (HeapTupleHeaderIsUpdate(new_tuple->t_data) &&
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(new_tuple->t_data),
state->rs_oldest_xmin))
{
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8418096..c26800d 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1169,11 +1169,22 @@ DefineEnum(CreateEnumStmt *stmt)
* Adds a new label to an existing enum.
*/
void
-AlterEnum(AlterEnumStmt *stmt)
+AlterEnum(AlterEnumStmt *stmt, bool toplevel)
{
Oid enum_type_oid;
TypeName *typename;
HeapTuple tup;
+ bool in_transaction;
+
+ /*
+ * When executed inside a transaction we need to run some extra checks to
+ * make sure its safe to alter the enum. It is only so if we can be sure
+ * the new value will not end up in an index thats still there after an
+ * abort of this transaction. The only easily detectable case of this is
+ * that the type were adding a value to was also created in this
+ * transaction.
+ */
+ in_transaction = !toplevel || IsTransactionBlock() || IsSubTransaction();
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(stmt->typeName);
@@ -1183,12 +1194,33 @@ AlterEnum(AlterEnumStmt *stmt)
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
+ /*
+ * We check whether the type was created in the same transaction by
+ * examining its xmin and checking the tuple was freshly inserted and not
+ * updated. This disallows some valid sequences like CREATE TYPE ... AS
+ * ENUM ...; ALTER TYPE ... OWNER TO ...; ALTER TYPE .. ADD VALUE ...; but
+ * thats acceptable because this is a special case to support pg_dump's
+ * --binary-upgrade mode which, err, luckily, doesn't do the above.
+ */
+ if (in_transaction &&
+ TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tup->t_data)) &&
+ !HeapTupleHeaderIsUpdate(tup->t_data))
+ {
+ /* good to go, all safe */
+ }
+ /*
+ * Emit error message *if* in transaction and our safety criterions are
+ * missed.
+ */
+ else
+ PreventTransactionChain(toplevel, "ALTER TYPE ... ADD");
+
/* Check it's an enum and check user has permission to ALTER the enum */
checkEnumOwner(tup);
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
- stmt->newValNeighbor, stmt->newValIsAfter,
+ stmt->newValNeighbor, stmt->newValIsAfter,
stmt->skipIfExists);
ReleaseSysCache(tup);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 491bd29..bebc81f 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -974,12 +974,16 @@ standard_ProcessUtility(Node *parsetree,
EventTriggerDDLCommandStart(parsetree);
/*
- * 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.
+ * We normally 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. We can tolerate
+ * that case though if the type itself was also only created in
+ * this transaction because no indexes containing this type will
+ * survive an abort anyway. We detect whether the type was created
+ * in this TXN inside AlterEnum, thats why it gets passed
+ * isTopLevel.
*/
- PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD");
- AlterEnum((AlterEnumStmt *) parsetree);
+ AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
break;
case T_ViewStmt: /* CREATE VIEW */
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 7abe3e6..266791f 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -313,6 +313,11 @@ do { \
*((Oid *) ((char *)(tup) + (tup)->t_hoff - sizeof(Oid))) = (oid); \
} while (0)
+#define HeapTupleHeaderIsUpdate(tup) \
+( \
+ (tup)->t_infomask & HEAP_UPDATED \
+)
+
/*
* Note that we stop considering a tuple HOT-updated as soon as it is known
* aborted or the would-be updating transaction is known aborted. For best
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index 2351024..792b146 100644
--- a/src/include/commands/typecmds.h
+++ b/src/include/commands/typecmds.h
@@ -26,7 +26,7 @@ extern void RemoveTypeById(Oid typeOid);
extern void DefineDomain(CreateDomainStmt *stmt);
extern void DefineEnum(CreateEnumStmt *stmt);
extern void DefineRange(CreateRangeStmt *stmt);
-extern void AlterEnum(AlterEnumStmt *stmt);
+extern void AlterEnum(AlterEnumStmt *stmt, bool toplevel);
extern Oid DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index ed729dd..7d08c7d 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -556,6 +556,30 @@ ERROR: foreign key constraint "enumtest_bogus_child_parent_fkey" cannot be impl
DETAIL: Key columns "parent" and "id" are of incompatible types: bogus and rainbow.
DROP TYPE bogus;
--
+-- check transactional behaviour of ALTER TYPE ... ADD VALUE
+--
+CREATE TYPE bogus AS ENUM();
+-- check that we can't add new values to existing enums in a transaction
+BEGIN;
+ALTER TYPE bogus ADD VALUE 'bad';
+ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
+COMMIT;
+-- check that we recognize the case where the enum already existed but was
+-- modified in the same txn
+BEGIN;
+ALTER TYPE bogus RENAME TO bogon;
+ALTER TYPE bogon ADD VALUE 'bad';
+ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
+ROLLBACK;
+DROP TYPE bogus;
+-- check that we *can* add new values to existing enums in a transaction, if
+-- the type is new as well
+BEGIN;
+CREATE TYPE bogus AS ENUM();
+ALTER TYPE bogus ADD VALUE 'good';
+ALTER TYPE bogus ADD VALUE 'ugly';
+ROLLBACK;
+--
-- Cleanup
--
DROP TABLE enumtest_child;
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 130a723..f3d55a6 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -258,6 +258,34 @@ CREATE TABLE enumtest_bogus_child(parent bogus REFERENCES enumtest_parent);
DROP TYPE bogus;
--
+-- check transactional behaviour of ALTER TYPE ... ADD VALUE
+--
+CREATE TYPE bogus AS ENUM();
+
+-- check that we can't add new values to existing enums in a transaction
+BEGIN;
+ALTER TYPE bogus ADD VALUE 'bad';
+COMMIT;
+
+-- check that we recognize the case where the enum already existed but was
+-- modified in the same txn
+BEGIN;
+ALTER TYPE bogus RENAME TO bogon;
+ALTER TYPE bogon ADD VALUE 'bad';
+ROLLBACK;
+
+DROP TYPE bogus;
+
+-- check that we *can* add new values to existing enums in a transaction, if
+-- the type is new as well
+BEGIN;
+CREATE TYPE bogus AS ENUM();
+ALTER TYPE bogus ADD VALUE 'good';
+ALTER TYPE bogus ADD VALUE 'ugly';
+ROLLBACK;
+
+
+--
-- Cleanup
--
DROP TABLE enumtest_child;
--
1.7.10.4
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@2ndquadrant.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2012-12-01 14:31:03 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@2ndquadrant.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers