From 7b90469b71761d240bf5efe3ad5bbd228429278e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Dec 2012 14:27:30 -0500 Subject: [PATCH] Allow adding values to an enum type created in the current transaction. Normally it is unsafe to allow ALTER TYPE ADD VALUE in a transaction block, because instances of the value could be added to indexes later in the same transaction, and then they would still be accessible even if the transaction rolls back. However, we can allow this if the enum type itself was created in the current transaction, because then any such indexes would have to go away entirely on rollback. The reason for allowing this is to support pg_upgrade's new usage of pg_restore --single-transaction: in --binary-upgrade mode, pg_dump emits enum types as a succession of ALTER TYPE ADD VALUE commands so that it can preserve the values' OIDs. The support is a bit limited, so we'll leave it undocumented. Andres Freund --- src/backend/commands/typecmds.c | 23 +++++++++++++++++++++-- src/backend/tcop/utility.c | 9 +-------- src/include/commands/typecmds.h | 2 +- src/test/regress/expected/enum.out | 24 ++++++++++++++++++++++++ src/test/regress/sql/enum.sql | 27 +++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 8418096c30..36de6d7e28 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -1169,7 +1169,7 @@ DefineEnum(CreateEnumStmt *stmt) * Adds a new label to an existing enum. */ void -AlterEnum(AlterEnumStmt *stmt) +AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) { Oid enum_type_oid; TypeName *typename; @@ -1183,12 +1183,31 @@ AlterEnum(AlterEnumStmt *stmt) if (!HeapTupleIsValid(tup)) elog(ERROR, "cache lookup failed for type %u", enum_type_oid); + /* + * Ordinarily we disallow adding values within transaction blocks, because + * we can't cope with enum OID values getting into indexes and then having + * their defining pg_enum entries go away. However, it's okay if the enum + * type was created in the current transaction, since then there can be + * no such indexes that wouldn't themselves go away on rollback. (We + * support this case because pg_dump --binary-upgrade needs it.) We test + * this by seeing if the pg_type row has xmin == current XID and is not + * HEAP_UPDATED. If it is HEAP_UPDATED, we can't be sure whether the + * type was created or only modified in this xact. So we are disallowing + * some cases that could theoretically be safe; but fortunately pg_dump + * only needs the simplest case. + */ + if (HeapTupleHeaderGetXmin(tup->t_data) == GetCurrentTransactionId() && + !(tup->t_data->t_infomask & HEAP_UPDATED)) + /* safe to do inside transaction block */ ; + else + PreventTransactionChain(isTopLevel, "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 491bd29a1c..a42b8e9b53 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -972,14 +972,7 @@ standard_ProcessUtility(Node *parsetree, case T_AlterEnumStmt: /* ALTER TYPE (enum) */ if (isCompleteQuery) 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. - */ - 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 2351024c22..e87ca90089 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 isTopLevel); 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 ed729dddc3..36826428a0 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('good'); +-- 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 current 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 130a723f69..88a835e8aa 100644 --- a/src/test/regress/sql/enum.sql +++ b/src/test/regress/sql/enum.sql @@ -257,6 +257,33 @@ CREATE TYPE bogus AS ENUM('good', 'bad', 'ugly'); 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('good'); + +-- 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 current 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 --