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;
+ 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
