]> granicus.if.org Git - postgresql/commitdiff
Revert to 9.6 treatment of ALTER TYPE enumtype ADD VALUE.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 27 Sep 2017 20:14:37 +0000 (16:14 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 27 Sep 2017 20:14:43 +0000 (16:14 -0400)
This reverts commit 15bc038f9, along with the followon commits 1635e80d3
and 984c92074 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

doc/src/sgml/ref/alter_type.sgml
src/backend/access/transam/xact.c
src/backend/catalog/pg_enum.c
src/backend/commands/typecmds.c
src/backend/tcop/utility.c
src/backend/utils/adt/enum.c
src/backend/utils/errcodes.txt
src/include/catalog/pg_enum.h
src/include/commands/typecmds.h
src/test/regress/expected/enum.out
src/test/regress/sql/enum.sql

index 7e2258e1e34d366b67c1c5fbf0aef5a183636f97..4027c1b8f7d72c108d13befbade3d610a1384d8b 100644 (file)
@@ -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>
index 52408fc6b06d3e700e477ca6177116ed959db97a..93dca7a72af59be806e8d47ad30292a764b8bc0e 100644 (file)
@@ -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();
index 0f7b36e11d88b1e98b301e9194ecc1fadb951d92..fe61d4dacccd218366ae0bb9be9e1dc32ea1d680 100644 (file)
@@ -28,8 +28,6 @@
 #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 @@ restart:
        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.
index 7ed16aeff460eba0c90224d6e5c3f1c1fdbc2f1a..4c490ed5c1bba46b1f22d0aef9eabc3169f38d9b 100644 (file)
@@ -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;
 }
 
index 5c69ecf0f7503a1c944b3d932a9bc8d515590b2c..82a707af7b8b2d7e7e1942176b53966387c450c7 100644 (file)
@@ -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 */
index c0124f497e472503652f9a15ac868a39b206a6a8..048a08dd852f56026a4f41308b622862a166cc57 100644 (file)
@@ -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;
index 4f354717628d4e56e329f03bc17b99c3c236f6ff..76fe79eac05fcb62d3fbed30b5429249deb29c70 100644 (file)
@@ -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
 
index dff3d2f481ca2d7de9eea98bb0fdb2e835dbf671..5938ba5cac307d2e4d2095526f1491228015d926 100644 (file)
@@ -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 */
index 34f6fe328fe1caf6a1ce86fa496168c6dbe59232..8f3fc6553637f464164ccb35659e887e5af1b4e7 100644 (file)
@@ -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);
 
index 4f839ce02797c7c0c7136bdd81105f21d12287f0..a0b81608a1bb07613f14df43e70ba9de15687d69 100644 (file)
@@ -581,60 +581,19 @@ ERROR:  enum label "green" already exists
 -- check transactional behaviour of ALTER TYPE ... ADD VALUE
 --
 CREATE TYPE bogus AS ENUM('good');
--- check that we can add new values to existing enums in a transaction
--- but we can't use them
+-- check that we can't add new values to existing enums in a transaction
 BEGIN;
-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;
+ALTER TYPE bogus ADD VALUE 'bad';
+ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
 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; this should not be considered safe
+-- modified in the current txn
 BEGIN;
 ALTER TYPE bogus RENAME TO bogon;
 ALTER TYPE bogon ADD VALUE 'bad';
-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.
+ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
 ROLLBACK;
--- but a renamed value is safe to use later in same transaction
+-- but ALTER TYPE RENAME VALUE is safe in a transaction
 BEGIN;
 ALTER TYPE bogus RENAME VALUE 'good' to 'bad';
 SELECT 'bad'::bogus;
@@ -645,27 +604,12 @@ SELECT 'bad'::bogus;
 
 ROLLBACK;
 DROP TYPE bogus;
--- check that values created during CREATE TYPE can be used in any case
-BEGIN;
-CREATE TYPE bogus AS ENUM('good','bad','ugly');
-ALTER TYPE bogus RENAME TO bogon;
-select enum_range(null::bogon);
-   enum_range    
------------------
- {good,bad,ugly}
-(1 row)
-
-ROLLBACK;
--- 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
+-- 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('good');
-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.
+CREATE TYPE bogus AS ENUM();
+ALTER TYPE bogus ADD VALUE 'good';
+ALTER TYPE bogus ADD VALUE 'ugly';
 ROLLBACK;
 --
 -- Cleanup
index 6affd0d1ebec573a90428950b07ee3044a70551a..7b68b2fe3768f34850d1f7fd50dfe9d7d05426ac 100644 (file)
@@ -273,34 +273,19 @@ ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
 --
 CREATE TYPE bogus AS ENUM('good');
 
--- check that we can add new values to existing enums in a transaction
--- but we can't use them
+-- check that we can't add new values to existing enums in a transaction
 BEGIN;
-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;
+ALTER TYPE bogus ADD VALUE 'bad';
 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; this should not be considered safe
+-- modified in the current txn
 BEGIN;
 ALTER TYPE bogus RENAME TO bogon;
 ALTER TYPE bogon ADD VALUE 'bad';
-SELECT 'bad'::bogon;
 ROLLBACK;
 
--- but a renamed value is safe to use later in same transaction
+-- but ALTER TYPE RENAME VALUE is safe in a transaction
 BEGIN;
 ALTER TYPE bogus RENAME VALUE 'good' to 'bad';
 SELECT 'bad'::bogus;
@@ -308,21 +293,12 @@ ROLLBACK;
 
 DROP TYPE bogus;
 
--- check that values created during CREATE TYPE can be used in any case
-BEGIN;
-CREATE TYPE bogus AS ENUM('good','bad','ugly');
-ALTER TYPE bogus RENAME TO bogon;
-select enum_range(null::bogon);
-ROLLBACK;
-
--- 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
+-- 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('good');
-ALTER TYPE bogus RENAME TO bogon;
-ALTER TYPE bogon ADD VALUE 'bad';
-ALTER TYPE bogon ADD VALUE 'ugly';
-select enum_range(null::bogon);  -- fails
+CREATE TYPE bogus AS ENUM();
+ALTER TYPE bogus ADD VALUE 'good';
+ALTER TYPE bogus ADD VALUE 'ugly';
 ROLLBACK;
 
 --