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

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8418096c308ab74e898afd9624a856099553ef38..36de6d7e28b1bf8c01e6d5edec5a0d62ed1fad68 100644 (file)
--- 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 491bd29a1c6f1f0ede999a7014da5422d75bc017..a42b8e9b535b8866678da484610eb242f2999a7f 100644 (file)
--- 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 2351024c2212199dac3563c222aa171a3dacc8cd..e87ca900892555011e3f171cc985414c466bec83 100644 (file)
--- 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 ed729dddc3f3fdfece86934d48c6645e480cef4a..36826428a07107b7289ce44ed608e4a15cbfcb93 100644 (file)
--- 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 130a723f698afce670bb2b1abd12b050524457d2..88a835e8aaca0d64c3eb77ac562080f11613c515 100644 (file)
--- 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
 --