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

Author: tglsfdc

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);

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 */

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);
 

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;

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
 --