Re: Alter or rename enum value

Andrew Dunstan <andrew@dunslane.net> writes:

> On 03/25/2016 04:13 AM, Matthias Kurz wrote:
>>
>> Hopefully at the commitfest at least the transaction limitation
>> will/could be tackled - that would help us a lot already.
>
> I don't believe anyone knows how to do that safely. Enums pose special
> problems here exactly because unlike all other types the set of valid
> values is mutable.

However, this problem (and the one described in the comments of
AlterEnum()) doesn't apply to altering the name, since that doesn't
affect the OID or the ordering.  Attached is version 2 of the patch,
which allows ALTER TYPE ... ALTER VALUE inside a transaction.

It still needs documentation, and possibly support for IF (NOT) EXISTS,
if people think that's useful.

>From 0a482f6d7434964ce51f66c260bde4a74dac4da0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 24 Mar 2016 17:50:58 +0000
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Unlike adding values, altering a value can be done in a transaction,
since it doesn't affect the OID values or ordering, so is safe to
rollback.

TODO: documentation, IF (NOT) EXISTS support(?)
---
 src/backend/catalog/pg_enum.c      | 88 ++++++++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c    | 50 ++++++++++++----------
 src/backend/parser/gram.y          | 14 ++++++
 src/include/catalog/pg_enum.h      |  2 +
 src/include/nodes/parsenodes.h     |  1 +
 src/test/regress/expected/enum.out | 58 +++++++++++++++++++++++++
 src/test/regress/sql/enum.sql      | 27 ++++++++++++
 7 files changed, 218 insertions(+), 22 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..1079e6c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,94 @@ restart:


 /*
+ * RenameEnumLabel
+ *        Add a new label to the enum set. By default it goes at
+ *        the end, but the user can choose to place it before or
+ *        after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+                const char *oldVal,
+                const char *newVal)
+{
+    Relation    pg_enum;
+    HeapTuple    old_tup = NULL;
+    HeapTuple    enum_tup;
+    CatCList   *list;
+    int            nelems;
+    int            nbr_index;
+    Form_pg_enum nbr_en;
+    bool        found_new = false;
+
+    /* check length of new label is ok */
+    if (strlen(newVal) > (NAMEDATALEN - 1))
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_NAME),
+                 errmsg("invalid enum label \"%s\"", newVal),
+                 errdetail("Labels must be %d characters or less.",
+                           NAMEDATALEN - 1)));
+
+    /*
+     * Acquire a lock on the enum type, which we won't release until commit.
+     * This ensures that two backends aren't concurrently modifying the same
+     * enum type.  Without that, we couldn't be sure to get a consistent view
+     * of the enum members via the syscache.  Note that this does not block
+     * other backends from inspecting the type; see comments for
+     * RenumberEnumType.
+     */
+    LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+    pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+    /* Get the list of existing members of the enum */
+    list = SearchSysCacheList1(ENUMTYPOIDNAME,
+                               ObjectIdGetDatum(enumTypeOid));
+    nelems = list->n_members;
+
+    /* Locate the element to rename and check if the new label is already in
+     * use.  The unique index on pg_enum would catch this anyway, but we
+     * prefer a friendlier error message.
+     */
+    for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+    {
+        enum_tup = &(list->members[nbr_index]->tuple);
+        nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+        if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+            old_tup = enum_tup;
+
+        if (strcmp(NameStr(nbr_en->enumlabel), newVal) == 0)
+            found_new = true;
+    }
+    if (!old_tup)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("\"%s\" is not an existing enum label",
+                        oldVal)));
+    if (found_new)
+        ereport(ERROR,
+                (errcode(ERRCODE_DUPLICATE_OBJECT),
+                 errmsg("enum label \"%s\" already exists",
+                        newVal)));
+
+    enum_tup = heap_copytuple(old_tup);
+    nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+    ReleaseCatCacheList(list);
+
+    /* Update new pg_enum entry */
+    namestrcpy(&nbr_en->enumlabel, newVal);
+    simple_heap_update(pg_enum, &enum_tup->t_self, enum_tup);
+    CatalogUpdateIndexes(pg_enum, enum_tup);
+    heap_freetuple(enum_tup);
+
+    /* Make the updates visible */
+    CommandCounterIncrement();
+
+    heap_close(pg_enum, RowExclusiveLock);
+}
+
+
+/*
  * 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
index 227d382..63f030b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1236,32 +1236,38 @@ 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");
+    if (!stmt->oldVal) {
+        /*
+         * 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);

-    /* Add the new label */
-    AddEnumLabel(enum_type_oid, stmt->newVal,
-                 stmt->newValNeighbor, stmt->newValIsAfter,
-                 stmt->skipIfExists);
+    if (stmt->oldVal)
+        /* Update the existing label */
+        RenameEnumLabel(enum_type_oid, stmt->oldVal, stmt->newVal);
+    else
+        /* Add the new label */
+        AddEnumLabel(enum_type_oid, stmt->newVal,
+                     stmt->newValNeighbor, stmt->newValIsAfter,
+                     stmt->skipIfExists);

     InvokeObjectPostAlterHook(TypeRelationId, enum_type_oid, 0);

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1273352..b7fea7a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5253,6 +5253,7 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = NULL;
                 n->newValIsAfter = true;
@@ -5263,6 +5264,7 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = $9;
                 n->newValIsAfter = false;
@@ -5273,12 +5275,24 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = $9;
                 n->newValIsAfter = true;
                 n->skipIfExists = $6;
                 $$ = (Node *) n;
             }
+         | ALTER TYPE_P any_name ALTER VALUE_P Sconst TO Sconst
+            {
+                AlterEnumStmt *n = makeNode(AlterEnumStmt);
+                n->typeName = $3;
+                n->oldVal = $6;
+                n->newVal = $8;
+                n->newValNeighbor = NULL;
+                n->newValIsAfter = true;
+                n->skipIfExists = false;
+                $$ = (Node *) n;
+            }
          ;

 opt_if_not_exists: IF_P NOT EXISTS              { $$ = true; }
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index dd32443..1a06482 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -67,5 +67,7 @@ extern void EnumValuesDelete(Oid enumTypeOid);
 extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
              const char *neighbor, bool newValIsAfter,
              bool skipIfExists);
+extern void RenameEnumLabel(Oid enumTypeOid, const char *oldVal,
+                            const char *newVal);

 #endif   /* PG_ENUM_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 8b958b4..0fc65d4 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2693,6 +2693,7 @@ typedef struct AlterEnumStmt
     NodeTag        type;
     List       *typeName;        /* qualified name (list of Value strings) */
     char       *newVal;            /* new enum value's name */
+    char       *oldVal;         /* old enum value's name when renaming */
     char       *newValNeighbor; /* neighboring enum value, if specified */
     bool        newValIsAfter;    /* place new enum value after neighbor? */
     bool        skipIfExists;    /* no error if label already exists */
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 1a61a5b..39bf239 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -556,6 +556,64 @@ CREATE TABLE enumtest_bogus_child(parent bogus REFERENCES enumtest_parent);
 ERROR:  foreign key constraint "enumtest_bogus_child_parent_fkey" cannot be implemented
 DETAIL:  Key columns "parent" and "id" are of incompatible types: bogus and rainbow.
 DROP TYPE bogus;
+-- check that we can rename a value
+ALTER TYPE rainbow ALTER VALUE 'red' TO 'crimson';
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+ enumlabel | enumsortorder
+-----------+---------------
+ crimson   |             1
+ orange    |             2
+ yellow    |             3
+ green     |             4
+ blue      |             5
+ purple    |             6
+(6 rows)
+
+-- check that renaming a non-existent value fails
+ALTER TYPE rainbow ALTER VALUE 'red' TO 'crimson';
+ERROR:  "red" is not an existing enum label
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green';
+ERROR:  enum label "green" already exists
+-- check that renaming a type in a transaction works
+BEGIN;
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'bleu';
+COMMIT;
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+ enumlabel | enumsortorder
+-----------+---------------
+ crimson   |             1
+ orange    |             2
+ yellow    |             3
+ green     |             4
+ bleu      |             5
+ purple    |             6
+(6 rows)
+
+-- check that rolling back a rename works
+BEGIN;
+ALTER TYPE rainbow ALTER VALUE 'bleu' TO 'blue';
+ROLLBACK;
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+ enumlabel | enumsortorder
+-----------+---------------
+ crimson   |             1
+ orange    |             2
+ yellow    |             3
+ green     |             4
+ bleu      |             5
+ purple    |             6
+(6 rows)
+
 --
 -- check transactional behaviour of ALTER TYPE ... ADD VALUE
 --
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 88a835e..0863df8 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -257,6 +257,33 @@ CREATE TYPE bogus AS ENUM('good', 'bad', 'ugly');
 CREATE TABLE enumtest_bogus_child(parent bogus REFERENCES enumtest_parent);
 DROP TYPE bogus;

+-- check that we can rename a value
+ALTER TYPE rainbow ALTER VALUE 'red' TO 'crimson';
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+-- check that renaming a non-existent value fails
+ALTER TYPE rainbow ALTER VALUE 'red' TO 'crimson';
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green';
+-- check that renaming a type in a transaction works
+BEGIN;
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'bleu';
+COMMIT;
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+-- check that rolling back a rename works
+BEGIN;
+ALTER TYPE rainbow ALTER VALUE 'bleu' TO 'blue';
+ROLLBACK;
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+
 --
 -- check transactional behaviour of ALTER TYPE ... ADD VALUE
 --
--
2.8.0.rc3


--
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen