Remove heuristic same-transaction test from check_safe_enum_use().

The blacklist mechanism added by the preceding commit directly fixes
most of the practical cases that the same-transaction test was meant
to cover.  What remains is use-cases like

	begin;
	create type e as enum('x');
	alter type e add value 'y';
	-- use 'y' somehow
	commit;

However, because the same-transaction test is heuristic, it fails on
small variants of that, such as renaming the type or changing its
owner.  Rather than try to explain the behavior to users, let's
remove it and just have a rule that the newly added value can't be
used before being committed, full stop.  Perhaps later it will be
worth the implementation effort and overhead to have a more accurate
test for type-was-created-in-this-transaction.  We'll wait for some
field experience with v10 before deciding to do that.

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
index 446e08b1759305b06599e0e558406d3b62355fc5..7e2258e1e34d366b67c1c5fbf0aef5a183636f97 100644 (file)
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -292,8 +292,7 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME VALUE <repla
   <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, except in the case
-   that the enum type itself was created earlier in the same transaction.
+   be used until after the transaction has been committed.
   </para>
 
   <para>

diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 401e7299fa4cb4747933f6101ecfd49b19e5ffd3..c0124f497e472503652f9a15ac868a39b206a6a8 100644 (file)
--- a/src/backend/utils/adt/enum.c
+++ b/src/backend/utils/adt/enum.c
@@ -48,7 +48,14 @@ static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
  * 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.)
+ * 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.
@@ -58,7 +65,6 @@ 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
@@ -85,40 +91,11 @@ check_safe_enum_use(HeapTuple enumval_tup)
    if (!EnumBlacklisted(HeapTupleGetOid(enumval_tup)))
        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.
     */
+   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",

diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 6bbe488736de448b65ea703fcfb1f28dd1cf3d1b..4f839ce02797c7c0c7136bdd81105f21d12287f0 100644 (file)
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -656,18 +656,16 @@ select enum_range(null::bogon);
 (1 row)
 
 ROLLBACK;
--- check that we can add new values to existing enums in a transaction
--- and use them, if the type is new as well
+-- ideally, we'd allow this usage; but it requires keeping track of whether
+-- the enum type was created in the current transaction, which is expensive
 BEGIN;
 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)
-
+ALTER TYPE bogus RENAME TO bogon;
+ALTER TYPE bogon ADD VALUE 'bad';
+ALTER TYPE bogon ADD VALUE 'ugly';
+select enum_range(null::bogon);  -- fails
+ERROR:  unsafe use of new value "bad" of enum type bogon
+HINT:  New enum values must be committed before they can be used.
 ROLLBACK;
 --
 -- Cleanup

diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index eb464a72c5c769aa2c26b5ac25746c080f10bf93..6affd0d1ebec573a90428950b07ee3044a70551a 100644 (file)
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -315,13 +315,14 @@ ALTER TYPE bogus RENAME TO bogon;
 select enum_range(null::bogon);
 ROLLBACK;
 
--- check that we can add new values to existing enums in a transaction
--- and use them, if the type is new as well
+-- ideally, we'd allow this usage; but it requires keeping track of whether
+-- the enum type was created in the current transaction, which is expensive
 BEGIN;
 CREATE TYPE bogus AS ENUM('good');
-ALTER TYPE bogus ADD VALUE 'bad';
-ALTER TYPE bogus ADD VALUE 'ugly';
-SELECT enum_range(null::bogus);
+ALTER TYPE bogus RENAME TO bogon;
+ALTER TYPE bogon ADD VALUE 'bad';
+ALTER TYPE bogon ADD VALUE 'ugly';
+select enum_range(null::bogon);  -- fails
 ROLLBACK;
 
 --