Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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 and...@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=crakedt=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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andrew Dunstan]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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 -1AtOEXAct  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
640001697.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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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 -1AtOEXAct  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
   640001697.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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Bruce Momjian br...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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
From 2839c7037d4ca8903a322aba5c399f2e54f2d63b Mon Sep 17 00:00:00 2001
From: Andres Freund and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Andres Freund and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andrew Dunstan]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Andrew Dunstan and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andrew Dunstan]

On 12/01/2012 12:06 PM, Tom Lane wrote:

Andrew Dunstan and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

On 2012-12-01 12:00:46 -0500, Tom Lane wrote:
 Andres Freund and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Andres Freund and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

On 2012-12-01 12:14:37 -0500, Tom Lane wrote:
 Andres Freund and...@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
From 7288d2cfdd7300bc665ecbfa43640814e665dad1 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@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;
+	boolin_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 

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

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 and...@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:

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

-- 
  Bruce Momjian  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

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 and...@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:

commandALTER 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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Andres Freund and...@2ndquadrant.com writes:
 On 2012-12-01 13:43:44 -0500, Bruce Momjian wrote:
 I believe this text in alter_type.sgml need updating:
 
 commandALTER 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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

Andres Freund and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Bruce Momjian]

On Sat, Dec  1, 2012 at 02:31:03PM -0500, Tom Lane wrote:
 Andres Freund and...@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  br...@momjian.ushttp://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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andres Freund]

On 2012-12-01 14:31:03 -0500, Tom Lane wrote:
 Andres Freund and...@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

Re: [HACKERS] --single-transaction hack to pg_upgrade does not work

[Andrew Dunstan]

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 and...@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=crakedt=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

[HACKERS] --single-transaction hack to pg_upgrade does not work

[Tom Lane]

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