Relax transactional restrictions on ALTER TYPE ... ADD VALUE.

tglsfdc · tglsfdc · commit 15bc038f9bcd · 2016-09-05T12:59:55.000-04:00
To prevent possibly breaking indexes on enum columns, we must keep
uncommitted enum values from getting stored in tables, unless we
can be sure that any such column is new in the current transaction.

Formerly, we enforced this by disallowing ALTER TYPE ... ADD VALUE
from being executed at all in a transaction block, unless the target
enum type had been created in the current transaction.  This patch
removes that restriction, and instead insists that an uncommitted enum
value can't be referenced unless it belongs to an enum type created
in the same transaction as the value.  Per discussion, this should be
a bit less onerous.  It does require each function that could possibly
return a new enum value to SQL operations to check this restriction,
but there aren't so many of those that this seems unmaintainable.

Andrew Dunstan and Tom Lane

Discussion: &lt;4075.1459088427@sss.pgh.pa.us&gt;
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
@@ -266,8 +266,10 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
   <title>Notes</title>
 
   <para>
-   <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
-   enum type) cannot be executed inside a transaction block.
+   If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
+   an enum type) is executed inside a transaction block, the new value cannot
+   be used until after the transaction has been committed, except in the case
+   that the enum type itself was created earlier in the same transaction.
   </para>
 
   <para>
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
@@ -1221,7 +1221,7 @@ DefineEnum(CreateEnumStmt *stmt)
  *		Adds a new label to an existing enum.
  */
 ObjectAddress
-AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
+AlterEnum(AlterEnumStmt *stmt)
 {
 	Oid			enum_type_oid;
 	TypeName   *typename;
@@ -1236,25 +1236,6 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
 	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);
 
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
@@ -1359,7 +1359,7 @@ ProcessUtilitySlow(Node *parsetree,
 				break;
 
 			case T_AlterEnumStmt:		/* ALTER TYPE (enum) */
-				address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
+				address = AlterEnum((AlterEnumStmt *) parsetree);
 				break;
 
 			case T_ViewStmt:	/* CREATE VIEW */
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
@@ -19,6 +19,7 @@
 #include "catalog/indexing.h"
 #include "catalog/pg_enum.h"
 #include "libpq/pqformat.h"
+#include "storage/procarray.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
@@ -31,6 +32,93 @@ static Oid	enum_endpoint(Oid enumtypoid, ScanDirection direction);
 static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
 
 
+/*
+ * Disallow use of an uncommitted pg_enum tuple.
+ *
+ * We need to make sure that uncommitted enum values don't get into indexes.
+ * If they did, and if we then rolled back the pg_enum addition, we'd have
+ * broken the index because value comparisons will not work reliably without
+ * an underlying pg_enum entry.  (Note that removal of the heap entry
+ * containing an enum value is not sufficient to ensure that it doesn't appear
+ * in upper levels of indexes.)  To do this we prevent an uncommitted row from
+ * being used for any SQL-level purpose.  This is stronger than necessary,
+ * since the value might not be getting inserted into a table or there might
+ * be no index on its column, but it's easy to enforce centrally.
+ *
+ * However, it's okay to allow use of uncommitted values belonging to enum
+ * types that were themselves created in the same transaction, because then
+ * any such index would also be new and would go away altogether on rollback.
+ * (This case is required by pg_upgrade.)
+ *
+ * This function needs to be called (directly or indirectly) in any of the
+ * functions below that could return an enum value to SQL operations.
+ */
+static void
+check_safe_enum_use(HeapTuple enumval_tup)
+{
+	TransactionId xmin;
+	Form_pg_enum en;
+	HeapTuple	enumtyp_tup;
+
+	/*
+	 * If the row is hinted as committed, it's surely safe.  This provides a
+	 * fast path for all normal use-cases.
+	 */
+	if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
+		return;
+
+	/*
+	 * Usually, a row would get hinted as committed when it's read or loaded
+	 * into syscache; but just in case not, let's check the xmin directly.
+	 */
+	xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
+	if (!TransactionIdIsInProgress(xmin) &&
+		TransactionIdDidCommit(xmin))
+		return;
+
+	/* It is a new enum value, so check to see if the whole enum is new */
+	en = (Form_pg_enum) GETSTRUCT(enumval_tup);
+	enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));
+	if (!HeapTupleIsValid(enumtyp_tup))
+		elog(ERROR, "cache lookup failed for type %u", en->enumtypid);
+
+	/*
+	 * We insist that the type have been created in the same (sub)transaction
+	 * as the enum value.  It would be safe to allow the type's originating
+	 * xact to be a subcommitted child of the enum value's xact, but not vice
+	 * versa (since we might now be in a subxact of the type's originating
+	 * xact, which could roll back along with the enum value's subxact).  The
+	 * former case seems a sufficiently weird usage pattern as to not be worth
+	 * spending code for, so we're left with a simple equality check.
+	 *
+	 * We also insist that the type's pg_type row not be HEAP_UPDATED.  If it
+	 * is, we can't tell whether the row was created or only modified in the
+	 * apparent originating xact, so it might be older than that xact.  (We do
+	 * not worry whether the enum value is HEAP_UPDATED; if it is, we might
+	 * think it's too new and throw an unnecessary error, but we won't allow
+	 * an unsafe case.)
+	 */
+	if (xmin == HeapTupleHeaderGetXmin(enumtyp_tup->t_data) &&
+		!(enumtyp_tup->t_data->t_infomask & HEAP_UPDATED))
+	{
+		/* same (sub)transaction, so safe */
+		ReleaseSysCache(enumtyp_tup);
+		return;
+	}
+
+	/*
+	 * There might well be other tests we could do here to narrow down the
+	 * unsafe conditions, but for now just raise an exception.
+	 */
+	ereport(ERROR,
+			(errcode(ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE),
+			 errmsg("unsafe use of new value \"%s\" of enum type %s",
+					NameStr(en->enumlabel),
+					format_type_be(en->enumtypid)),
+	 errhint("New enum values must be committed before they can be used.")));
+}
+
+
 /* Basic I/O support */
 
 Datum
@@ -59,6 +147,9 @@ enum_in(PG_FUNCTION_ARGS)
 						format_type_be(enumtypoid),
 						name)));
 
+	/* check it's safe to use in SQL */
+	check_safe_enum_use(tup);
+
 	/*
 	 * This comes from pg_enum.oid and stores system oids in user tables. This
 	 * oid must be preserved by binary upgrades.
@@ -124,6 +215,9 @@ enum_recv(PG_FUNCTION_ARGS)
 						format_type_be(enumtypoid),
 						name)));
 
+	/* check it's safe to use in SQL */
+	check_safe_enum_use(tup);
+
 	enumoid = HeapTupleGetOid(tup);
 
 	ReleaseSysCache(tup);
@@ -327,9 +421,16 @@ enum_endpoint(Oid enumtypoid, ScanDirection direction)
 
 	enum_tuple = systable_getnext_ordered(enum_scan, direction);
 	if (HeapTupleIsValid(enum_tuple))
+	{
+		/* check it's safe to use in SQL */
+		check_safe_enum_use(enum_tuple);
 		minmax = HeapTupleGetOid(enum_tuple);
+	}
 	else
+	{
+		/* should only happen with an empty enum */
 		minmax = InvalidOid;
+	}
 
 	systable_endscan_ordered(enum_scan);
 	index_close(enum_idx, AccessShareLock);
@@ -490,6 +591,9 @@ enum_range_internal(Oid enumtypoid, Oid lower, Oid upper)
 
 		if (left_found)
 		{
+			/* check it's safe to use in SQL */
+			check_safe_enum_use(enum_tuple);
+
 			if (cnt >= max)
 			{
 				max *= 2;
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
@@ -398,6 +398,7 @@ Section: Class 55 - Object Not In Prerequisite State
 55006    E    ERRCODE_OBJECT_IN_USE                                          object_in_use
 55P02    E    ERRCODE_CANT_CHANGE_RUNTIME_PARAM                              cant_change_runtime_param
 55P03    E    ERRCODE_LOCK_NOT_AVAILABLE                                     lock_not_available
+55P04    E    ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE                            unsafe_new_enum_value_usage
 
 Section: Class 57 - Operator Intervention
 
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
@@ -26,7 +26,7 @@ extern void RemoveTypeById(Oid typeOid);
 extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
 extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
 extern ObjectAddress DefineRange(CreateRangeStmt *stmt);
-extern ObjectAddress AlterEnum(AlterEnumStmt *stmt, bool isTopLevel);
+extern ObjectAddress AlterEnum(AlterEnumStmt *stmt);
 extern ObjectAddress 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
@@ -560,25 +560,72 @@ 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
+-- check that we can add new values to existing enums in a transaction
+-- but we can't use them
 BEGIN;
-ALTER TYPE bogus ADD VALUE 'bad';
-ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
+ALTER TYPE bogus ADD VALUE 'new';
+SAVEPOINT x;
+SELECT 'new'::bogus;  -- unsafe
+ERROR:  unsafe use of new value "new" of enum type bogus
+LINE 1: SELECT 'new'::bogus;
+               ^
+HINT:  New enum values must be committed before they can be used.
+ROLLBACK TO x;
+SELECT enum_first(null::bogus);  -- safe
+ enum_first 
+------------
+ good
+(1 row)
+
+SELECT enum_last(null::bogus);  -- unsafe
+ERROR:  unsafe use of new value "new" of enum type bogus
+HINT:  New enum values must be committed before they can be used.
+ROLLBACK TO x;
+SELECT enum_range(null::bogus);  -- unsafe
+ERROR:  unsafe use of new value "new" of enum type bogus
+HINT:  New enum values must be committed before they can be used.
+ROLLBACK TO x;
 COMMIT;
+SELECT 'new'::bogus;  -- now safe
+ bogus 
+-------
+ new
+(1 row)
+
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'bogus'::regtype
+ORDER BY 2;
+ enumlabel | enumsortorder 
+-----------+---------------
+ good      |             1
+ new       |             2
+(2 rows)
+
 -- check that we recognize the case where the enum already existed but was
--- modified in the current txn
+-- modified in the current txn; this should not be considered safe
 BEGIN;
 ALTER TYPE bogus RENAME TO bogon;
 ALTER TYPE bogon ADD VALUE 'bad';
-ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
+SELECT 'bad'::bogon;
+ERROR:  unsafe use of new value "bad" of enum type bogon
+LINE 1: SELECT 'bad'::bogon;
+               ^
+HINT:  New enum values must be committed before they can be used.
 ROLLBACK;
 DROP TYPE bogus;
--- check that we *can* add new values to existing enums in a transaction,
--- if the type is new as well
+-- check that we can add new values to existing enums in a transaction
+-- and use them, if the type is new as well
 BEGIN;
-CREATE TYPE bogus AS ENUM();
-ALTER TYPE bogus ADD VALUE 'good';
+CREATE TYPE bogus AS ENUM('good');
+ALTER TYPE bogus ADD VALUE 'bad';
 ALTER TYPE bogus ADD VALUE 'ugly';
+SELECT enum_range(null::bogus);
+   enum_range    
+-----------------
+ {good,bad,ugly}
+(1 row)
+
 ROLLBACK;
 --
 -- Cleanup
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
@@ -262,26 +262,42 @@ DROP TYPE bogus;
 --
 CREATE TYPE bogus AS ENUM('good');
 
--- check that we can't add new values to existing enums in a transaction
+-- check that we can add new values to existing enums in a transaction
+-- but we can't use them
 BEGIN;
-ALTER TYPE bogus ADD VALUE 'bad';
+ALTER TYPE bogus ADD VALUE 'new';
+SAVEPOINT x;
+SELECT 'new'::bogus;  -- unsafe
+ROLLBACK TO x;
+SELECT enum_first(null::bogus);  -- safe
+SELECT enum_last(null::bogus);  -- unsafe
+ROLLBACK TO x;
+SELECT enum_range(null::bogus);  -- unsafe
+ROLLBACK TO x;
 COMMIT;
+SELECT 'new'::bogus;  -- now safe
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'bogus'::regtype
+ORDER BY 2;
 
 -- check that we recognize the case where the enum already existed but was
--- modified in the current txn
+-- modified in the current txn; this should not be considered safe
 BEGIN;
 ALTER TYPE bogus RENAME TO bogon;
 ALTER TYPE bogon ADD VALUE 'bad';
+SELECT 'bad'::bogon;
 ROLLBACK;
 
 DROP TYPE bogus;
 
--- check that we *can* add new values to existing enums in a transaction,
--- if the type is new as well
+-- check that we can add new values to existing enums in a transaction
+-- and use them, if the type is new as well
 BEGIN;
-CREATE TYPE bogus AS ENUM();
-ALTER TYPE bogus ADD VALUE 'good';
+CREATE TYPE bogus AS ENUM('good');
+ALTER TYPE bogus ADD VALUE 'bad';
 ALTER TYPE bogus ADD VALUE 'ugly';
+SELECT enum_range(null::bogus);
 ROLLBACK;
 
 --
