Backport transactional enum patch (15bc038) from 10.0

Author: kelvich

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>

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

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

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;

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
 

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

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

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