Revert to 9.6 treatment of ALTER TYPE enumtype ADD VALUE.

tglsfdc · tglsfdc · commit 28e072707685 · 2017-09-27T16:14:43.000-04:00
This reverts commit 15bc038, along with the followon commits 1635e80 and 984c920 that tried to clean up the problems exposed by bug #14825. The result was incomplete because it failed to address parallel-query requirements. With 10.0 release so close upon us, now does not seem like the time to be adding more code to fix that. I hope we can un-revert this code and add the missing parallel query support during the v11 cycle. Back-patch to v10. Discussion: https://postgr.es/m/20170922185904.1448.16585@wrigleys.postgresql.org
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
@@ -290,9 +290,8 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME VALUE <repla
   <title>Notes</title>
 
   <para>
-   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.
+   <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
+   enum type) cannot be executed inside a transaction block.
   </para>
 
   <para>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
@@ -32,7 +32,6 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
-#include "catalog/pg_enum.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
 #include "commands/tablecmds.h"
@@ -2129,7 +2128,6 @@ CommitTransaction(void)
 	AtCommit_Notify();
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
-	AtEOXact_Enum();
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true, is_parallel_worker);
 	AtEOXact_SMgr();
@@ -2408,7 +2406,6 @@ PrepareTransaction(void)
 	/* PREPARE acts the same as COMMIT as far as GUC is concerned */
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
-	AtEOXact_Enum();
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true, false);
 	AtEOXact_SMgr();
@@ -2611,7 +2608,6 @@ AbortTransaction(void)
 
 		AtEOXact_GUC(false, 1);
 		AtEOXact_SPI(false);
-		AtEOXact_Enum();
 		AtEOXact_on_commit_actions(false);
 		AtEOXact_Namespace(false, is_parallel_worker);
 		AtEOXact_SMgr();
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
@@ -28,26 +28,13 @@
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
-#include "utils/hsearch.h"
-#include "utils/memutils.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
 
 
 /* Potentially set by pg_upgrade_support functions */
 Oid			binary_upgrade_next_pg_enum_oid = InvalidOid;
 
-/*
- * Hash table of enum value OIDs created during the current transaction by
- * AddEnumLabel.  We disallow using these values until the transaction is
- * committed; otherwise, they might get into indexes where we can't clean
- * them up, and then if the transaction rolls back we have a broken index.
- * (See comments for check_safe_enum_use() in enum.c.)  Values created by
- * EnumValuesCreate are *not* blacklisted; we assume those are created during
- * CREATE TYPE, so they can't go away unless the enum type itself does.
- */
-static HTAB *enum_blacklist = NULL;
-
 static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
 static int	sort_order_cmp(const void *p1, const void *p2);
 
@@ -473,24 +460,6 @@ AddEnumLabel(Oid enumTypeOid,
 	heap_freetuple(enum_tup);
 
 	heap_close(pg_enum, RowExclusiveLock);
-
-	/* Set up the blacklist hash if not already done in this transaction */
-	if (enum_blacklist == NULL)
-	{
-		HASHCTL		hash_ctl;
-
-		memset(&hash_ctl, 0, sizeof(hash_ctl));
-		hash_ctl.keysize = sizeof(Oid);
-		hash_ctl.entrysize = sizeof(Oid);
-		hash_ctl.hcxt = TopTransactionContext;
-		enum_blacklist = hash_create("Enum value blacklist",
-									 32,
-									 &hash_ctl,
-									 HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
-	}
-
-	/* Add the new value to the blacklist */
-	(void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL);
 }
 
 
@@ -578,39 +547,6 @@ RenameEnumLabel(Oid enumTypeOid,
 }
 
 
-/*
- * Test if the given enum value is on the blacklist
- */
-bool
-EnumBlacklisted(Oid enum_id)
-{
-	bool		found;
-
-	/* If we've made no blacklist table, all values are safe */
-	if (enum_blacklist == NULL)
-		return false;
-
-	/* Else, is it in the table? */
-	(void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found);
-	return found;
-}
-
-
-/*
- * Clean up enum stuff after end of top-level transaction.
- */
-void
-AtEOXact_Enum(void)
-{
-	/*
-	 * Reset the blacklist table, as all our enum values are now committed.
-	 * The memory will go away automatically when TopTransactionContext is
-	 * freed; it's sufficient to clear our pointer.
-	 */
-	enum_blacklist = NULL;
-}
-
-
 /*
  * RenumberEnumType
  *		Renumber existing enum elements to have sort positions 1..n.
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
@@ -1222,10 +1222,10 @@ DefineEnum(CreateEnumStmt *stmt)
 
 /*
  * AlterEnum
- *		Adds a new label to an existing enum.
+ *		ALTER TYPE on an enum.
  */
 ObjectAddress
-AlterEnum(AlterEnumStmt *stmt)
+AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
 {
 	Oid			enum_type_oid;
 	TypeName   *typename;
@@ -1243,8 +1243,6 @@ AlterEnum(AlterEnumStmt *stmt)
 	/* Check it's an enum and check user has permission to ALTER the enum */
 	checkEnumOwner(tup);
 
-	ReleaseSysCache(tup);
-
 	if (stmt->oldVal)
 	{
 		/* Rename an existing label */
@@ -1253,6 +1251,27 @@ AlterEnum(AlterEnumStmt *stmt)
 	else
 	{
 		/* Add a new label */
+
+		/*
+		 * 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");
+
 		AddEnumLabel(enum_type_oid, stmt->newVal,
 					 stmt->newValNeighbor, stmt->newValIsAfter,
 					 stmt->skipIfNewValExists);
@@ -1262,6 +1281,8 @@ AlterEnum(AlterEnumStmt *stmt)
 
 	ObjectAddressSet(address, TypeRelationId, enum_type_oid);
 
+	ReleaseSysCache(tup);
+
 	return address;
 }
 
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
@@ -1412,7 +1412,7 @@ ProcessUtilitySlow(ParseState *pstate,
 				break;
 
 			case T_AlterEnumStmt:	/* ALTER TYPE (enum) */
-				address = AlterEnum((AlterEnumStmt *) parsetree);
+				address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
 				break;
 
 			case T_ViewStmt:	/* CREATE VIEW */
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
@@ -19,7 +19,6 @@
 #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"
@@ -32,79 +31,6 @@ 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.
- * We don't implement that fully right now, but we do allow free use of enum
- * values created during CREATE TYPE AS ENUM, which are surely of the same
- * lifespan as the enum type.  (This case is required by "pg_restore -1".)
- * Values added by ALTER TYPE ADD VALUE are currently restricted, but could
- * be allowed if the enum type could be proven to have been created earlier
- * in the same transaction.  (Note that comparing tuple xmins would not work
- * for that, because the type tuple might have been updated in the current
- * transaction.  Subtransactions also create hazards to be accounted for.)
- *
- * 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;
-
-	/*
-	 * 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;
-
-	/*
-	 * Check if the enum value is blacklisted.  If not, it's safe, because it
-	 * was made during CREATE TYPE AS ENUM and can't be shorter-lived than its
-	 * owning type.  (This'd also be false for values made by other
-	 * transactions; but the previous tests should have handled all of those.)
-	 */
-	if (!EnumBlacklisted(HeapTupleGetOid(enumval_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.
-	 */
-	en = (Form_pg_enum) GETSTRUCT(enumval_tup);
-	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
@@ -133,9 +59,6 @@ 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.
@@ -201,9 +124,6 @@ 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);
@@ -411,16 +331,9 @@ 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);
@@ -581,9 +494,6 @@ 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
@@ -400,7 +400,6 @@ 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/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
@@ -69,7 +69,5 @@ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
 			 bool skipIfExists);
 extern void RenameEnumLabel(Oid enumTypeOid,
 				const char *oldVal, const char *newVal);
-extern bool EnumBlacklisted(Oid enum_id);
-extern void AtEOXact_Enum(void);
 
 #endif							/* PG_ENUM_H */
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);
+extern ObjectAddress AlterEnum(AlterEnumStmt *stmt, bool isTopLevel);
 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
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
