Re: Alter or rename enum value

Matthias Kurz <m.kurz@irregular.at> writes:

[altering and dropping enum values]

>>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> > On 03/09/2016 11:07 AM, Tom Lane wrote:
>>> >> I have a vague recollection that we discussed this at the time the enum
>>> >> stuff went in, and there are concurrency issues?  Don't recall details
>>> >> though.
>>>
>>> > Rings a vague bell, but should it be any worse than adding new labels?
>>>
>>> I think what I was recalling is the hazards discussed in the comments for
>>> RenumberEnumType.  However, the problem there is that a backend could make
>>> inconsistent ordering decisions due to seeing two different pg_enum rows
>>> under different snapshots.  Updating a single row to change its name
>>> doesn't seem to have a comparable hazard, and it wouldn't affect ordering
>>> anyway.  So it's probably no worse than any other object-rename situation.
>>>
>>>                         regards, tom lane
>>>
>>
>>
> Is there a way or a procedure we can go through to make the these ALTER
> TYPE enhancements a higher priority? How do you choose which
> features/enhancements to implement (next)?

I was bored and thought "how hard could it be?", and a few hours'
hacking later, I have something that seems to work.  It doesn't do IF
NOT EXISTS yet, and the error messaging could do with some improvement,
and there are no docs.  The patch is attached, as well as at
https://github.com/ilmari/postgres/commit/enum-alter-value

>From 1cb8feac0179eaa44720822ad84e146ec3ba67ff 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 '...'

Needs docs, and when altering a non-existent value to one that already
exists, it should probably complain about the former fact, not the
latter.
---
 src/backend/catalog/pg_enum.c      | 93 ++++++++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c    | 17 +++++--
 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 | 22 +++++++++
 src/test/regress/sql/enum.sql      | 11 +++++
 7 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..81e170c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,99 @@ 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    enum_tup;
+    CatCList   *list;
+    int            nelems;
+    int            nbr_index;
+    Form_pg_enum nbr_en;
+
+
+
+    /* 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);
+
+    /*
+     * Check if label is already in use.  The unique index on pg_enum would
+     * catch this anyway, but we prefer a friendlier error message.
+     */
+    enum_tup = SearchSysCache2(ENUMTYPOIDNAME,
+                               ObjectIdGetDatum(enumTypeOid),
+                               CStringGetDatum(newVal));
+    if (HeapTupleIsValid(enum_tup))
+    {
+        ReleaseSysCache(enum_tup);
+        ereport(ERROR,
+                (errcode(ERRCODE_DUPLICATE_OBJECT),
+                 errmsg("enum label \"%s\" already exists",
+                        newVal)));
+    }
+
+    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 */
+    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)
+            break;
+    }
+    if (nbr_index >= nelems)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("\"%s\" is not an existing enum label",
+                        oldVal)));
+
+    enum_tup = heap_copytuple(enum_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..1d6167e 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1253,15 +1253,22 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
         !(tup->t_data->t_infomask & HEAP_UPDATED))
          /* safe to do inside transaction block */ ;
     else
-        PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD");
+        PreventTransactionChain(isTopLevel,
+                                stmt->oldVal
+                                ? "ALTER TYPE ... ALTER VALUE"
+                                : "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..9801e73 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..fed9c8b 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -556,6 +556,28 @@ 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 'ruby';
+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 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..f6d026f 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -257,6 +257,17 @@ 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 'ruby';
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green';
+
 --
 -- check transactional behaviour of ALTER TYPE ... ADD VALUE
 --
--
2.8.0.rc3



--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."                              -- Skud's Meta-Law