Re: [PATCH] Alter or rename enum value - Mailing list pgsql-hackers

Emre Hasegeli <emre@hasegeli.com> writes:

>> Bottom line here is that I'd rather commit ALTER TYPE RENAME VALUE with
>> no EXISTS features and then see it accrete those features together with
>> other types of RENAME, when and if there's a will to make that happen.
>
> This sounds like a good conclusion to me.

Works for me. I mainly added the IF [NOT] EXISTS support to be
consistent with ADD VALUE, and because I like idempotency, but I'm not
married to it.

Here is version 6 of the patch, which just adds RENAME VALUE with no IF
[NOT] EXISTS, rebased onto current master (particularly the
transactional ADD VALUE patch).

>From e4ee07d53bbab5ee434a12a2f30a9ac9bb5c89d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 6 Sep 2016 14:38:06 +0100
Subject: [PATCH 1/2] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20V?=
 =?UTF-8?q?ALUE=20=E2=80=A6=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Unlike adding values to an enum, altering an existing value doesn't
affect the OID values or ordering, so can be done in a transaction
regardless of whether the enum was created in the same transaction.
---
 doc/src/sgml/ref/alter_type.sgml   | 18 +++++++-
 src/backend/catalog/pg_enum.c      | 91 ++++++++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c    | 12 +++--
 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 +++++++++++
 8 files changed, 217 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index aec73f6..bdc2fa2 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -29,6 +29,7 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <r
 ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable
class="PARAMETER">new_name</replaceable>
 ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable
class="PARAMETER">new_schema</replaceable>
 ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT EXISTS ] <replaceable
class="PARAMETER">new_enum_value</replaceable>[ { BEFORE | AFTER } <replaceable
class="PARAMETER">existing_enum_value</replaceable>] 
+ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME VALUE <replaceable
class="PARAMETER">existing_enum_value</replaceable>TO <replaceable class="PARAMETER">new_enum_value</replaceable> 

 <phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>

@@ -123,6 +124,17 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
     </listitem>
    </varlistentry>

+   <varlistentry>
+    <term><literal>RENAME VALUE TO</literal></term>
+    <listitem>
+     <para>
+      This form renames a value in an enum type.
+      The value's place in the enum's ordering is not affected.
+      An error will occur if the old value is not already present or the new value is.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>CASCADE</literal></term>
     <listitem>
@@ -241,7 +253,8 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
       <term><replaceable class="PARAMETER">new_enum_value</replaceable></term>
       <listitem>
        <para>
-        The new value to be added to an enum type's list of values.
+        The new value to be added to an enum type's list of values,
+        or to rename an existing one to.
         Like all enum literals, it needs to be quoted.
        </para>
       </listitem>
@@ -252,7 +265,8 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
       <listitem>
        <para>
         The existing enum value that the new value should be added immediately
-        before or after in the enum type's sort ordering.
+        before or after in the enum type's sort ordering,
+        or renamed from.
         Like all enum literals, it needs to be quoted.
        </para>
       </listitem>
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index c66f963..2e48dfe 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -465,6 +465,97 @@ AddEnumLabel(Oid enumTypeOid,
 }


+/*
+ * RenameEnumLabel
+ *        Rename a label in an enum set.
+ */
+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 6cc7106..2807219 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1241,10 +1241,14 @@ AlterEnum(AlterEnumStmt *stmt)
     /* 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 b69a77a..21fc2f1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5257,6 +5257,7 @@ enum_val_list:    Sconst
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = NULL;
                 n->newValIsAfter = true;
@@ -5267,6 +5268,7 @@ enum_val_list:    Sconst
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = $9;
                 n->newValIsAfter = false;
@@ -5277,12 +5279,24 @@ enum_val_list:    Sconst
             {
                 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 RENAME 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..b3e53ac 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 3716c2e..15380af 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2709,6 +2709,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 d4a45a3..f288c79 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 RENAME 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 RENAME VALUE 'red' TO 'crimson';
+ERROR:  "red" is not an existing enum label
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
+ERROR:  enum label "green" already exists
+-- check that renaming a value in a transaction works
+BEGIN;
+ALTER TYPE rainbow RENAME 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 RENAME 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 d25e8de..160cc3d 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 RENAME 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 RENAME VALUE 'red' TO 'crimson';
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
+-- check that renaming a value in a transaction works
+BEGIN;
+ALTER TYPE rainbow RENAME 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 RENAME 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.9.3


--
"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