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

Emre Hasegeli <emre@hasegeli.com> writes:

>> Here is v4, which changes the command from ALTER VALUE to RENAME VALUE,
>> for consistency with RENAME ATTRIBUTE.
>
> It looks like we always use "ALTER ... RENAME ... old_name TO
> new_name" syntax, so it is better that way.  I have noticed that all
> the other RENAMEs go through ExecRenameStmt().  We better do the same.

That would require changing it from an AlterEnumStmt to a RenameStmt
instead.  Those look to me like they're for renaming SQL objects,
i.e. things addressed by identifiers, whereas enum labels are just
strings.  Looking at the implementation of a few of the functions called
by ExecRenameStmt(), they have very little in common with
RenameEnumLabel() logic-wise.

>> +    int         nbr_index;
>> +    Form_pg_enum nbr_en;
>
> What is "nbr"?  Why not calling them something like "i" and "val"?

This is the same naming as used in AddEnumLabel(), which I used as a
guide.

>> +   /* Locate the element to rename and check if the new label is already in
>
> The project style for multi-line commands is to leave the first line
> of the comment block empty.
>
>> +        if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
>
> I found namestrcmp() for this.

Thanks, fixed.  This again came from using AddEnumLabel() as an example,
which should probably be fixed separately.

>
>> +    }
>> +    if (!old_tup)
>
> I would leave a space in between.
>
>> +        ReleaseCatCacheList(list);
>> +        heap_close(pg_enum, RowExclusiveLock);
>
> Maybe we better release them before reporting error, too.  I would
> release the list after the loop, close the heap before ereport().

As Tom said, this gets released automatically on error, and is again
similar to how AddEnumLabel() does it.

Here is an updated patch.

>From 542b20b3a0f82d07203035aa853bae2ddccb6af3 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] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20VALUE?=
 =?UTF-8?q?=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.

IF EXISTS and IF NOT EXISTS are supported for ignoring the non-existence
of the old value or the existance of the new one, respectively.
---
 doc/src/sgml/ref/alter_type.sgml   |  24 +++++++-
 src/backend/catalog/pg_enum.c      | 117 +++++++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c    |  52 ++++++++++-------
 src/backend/parser/gram.y          |  18 ++++++
 src/include/catalog/pg_enum.h      |   3 +
 src/include/nodes/parsenodes.h     |   2 +
 src/test/regress/expected/enum.out |  74 +++++++++++++++++++++++
 src/test/regress/sql/enum.sql      |  35 +++++++++++
 8 files changed, 301 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..9f0ca8f 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 [ IF EXISTS ] <replaceable
class="PARAMETER">existing_enum_value</replaceable>TO <replaceable class="PARAMETER">new_enum_value</replaceable> [ IF
NOTEXISTS ] 

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

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

    <varlistentry>
+    <term><literal>RENAME VALUE [ IF EXISTS ] TO [ IF NOT EXISTS ]</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.
+     </para>
+     <para>
+      If <literal>IF EXISTS</literal> or <literal>IF NOT EXISTS</literal> is
+      specified, it is not an error if the type doesn't contain the old value
+      or already contains the new value, respectively: a notice is issued but
+      no other action is taken.  Otherwise, 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>
      <para>
@@ -241,7 +259,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 +271,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 af89daa..1920138 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,123 @@ restart:


 /*
+ * RenameEnumLabel
+ *        Rename a label in an enum set.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+                const char *oldVal,
+                const char *newVal,
+                bool skipIfNotExists,
+                bool skipIfExists)
+{
+    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 (namestrcmp(&(nbr_en->enumlabel), oldVal) == 0)
+            old_tup = enum_tup;
+
+        if (namestrcmp(&(nbr_en->enumlabel), newVal) == 0)
+            found_new = true;
+    }
+
+    if (!old_tup)
+    {
+        if (skipIfNotExists)
+        {
+            ereport(NOTICE,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("\"%s\" is not an existing enum label, skipping",
+                            oldVal)));
+            ReleaseCatCacheList(list);
+            heap_close(pg_enum, RowExclusiveLock);
+            return;
+        }
+        else
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("\"%s\" is not an existing enum label",
+                            oldVal)));
+    }
+
+    if (found_new)
+    {
+        if (skipIfExists)
+        {
+            ereport(NOTICE,
+                    (errcode(ERRCODE_DUPLICATE_OBJECT),
+                     errmsg("enum label \"%s\" already exists, skipping",
+                            newVal)));
+            ReleaseCatCacheList(list);
+            heap_close(pg_enum, RowExclusiveLock);
+            return;
+        }
+        else
+            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 ce04211..cd75226 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1236,32 +1236,40 @@ 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,
+                        stmt->skipIfNotExists, stmt->skipIfExists);
+    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 cb5cfc4..e5c1703 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5257,9 +5257,11 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = NULL;
                 n->newValIsAfter = true;
+                n->skipIfNotExists = false;
                 n->skipIfExists = $6;
                 $$ = (Node *) n;
             }
@@ -5267,9 +5269,11 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = $9;
                 n->newValIsAfter = false;
+                n->skipIfNotExists = false;
                 n->skipIfExists = $6;
                 $$ = (Node *) n;
             }
@@ -5277,12 +5281,26 @@ AlterEnumStmt:
             {
                 AlterEnumStmt *n = makeNode(AlterEnumStmt);
                 n->typeName = $3;
+                n->oldVal = NULL;
                 n->newVal = $7;
                 n->newValNeighbor = $9;
                 n->newValIsAfter = true;
+                n->skipIfNotExists = false;
                 n->skipIfExists = $6;
                 $$ = (Node *) n;
             }
+         | ALTER TYPE_P any_name RENAME VALUE_P opt_if_exists Sconst TO Sconst opt_if_not_exists
+            {
+                AlterEnumStmt *n = makeNode(AlterEnumStmt);
+                n->typeName = $3;
+                n->oldVal = $7;
+                n->newVal = $9;
+                n->newValNeighbor = NULL;
+                n->newValIsAfter = true;
+                n->skipIfNotExists = $6;
+                n->skipIfExists = $10;
+                $$ = (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..882432a 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -67,5 +67,8 @@ 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,
+                            bool skipIfNotExists, bool skipIfExists);

 #endif   /* PG_ENUM_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1481fff..6764d11 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2708,9 +2708,11 @@ 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 */
+    bool        skipIfNotExists;/* no error if label doesn't already exist */
 } AlterEnumStmt;

 /* ----------------------
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 1a61a5b..7dfb178 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -556,6 +556,79 @@ 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
+CREATE TABLE enumtest_default (colour rainbow DEFAULT 'red');
+INSERT INTO enumtest_default DEFAULT VALUES;
+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)
+
+INSERT INTO enumtest_default DEFAULT VALUES;
+SELECT * FROM enumtest_default;
+ colour
+---------
+ crimson
+ crimson
+(2 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 IF (NOT) EXISTS
+ALTER TYPE rainbow RENAME VALUE IF EXISTS 'blarm' TO 'fleem';
+NOTICE:  "blarm" is not an existing enum label, skipping
+ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green' IF NOT EXISTS;
+NOTICE:  enum label "green" already exists, skipping
+-- 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
 --
@@ -585,6 +658,7 @@ ROLLBACK;
 --
 DROP TABLE enumtest_child;
 DROP TABLE enumtest_parent;
+DROP TABLE enumtest_default;
 DROP TABLE enumtest;
 DROP TYPE rainbow;
 --
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 88a835e..916e13d 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -257,6 +257,40 @@ 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
+CREATE TABLE enumtest_default (colour rainbow DEFAULT 'red');
+INSERT INTO enumtest_default DEFAULT VALUES;
+ALTER TYPE rainbow RENAME VALUE 'red' TO 'crimson';
+SELECT enumlabel, enumsortorder
+FROM pg_enum
+WHERE enumtypid = 'rainbow'::regtype
+ORDER BY 2;
+INSERT INTO enumtest_default DEFAULT VALUES;
+SELECT * FROM enumtest_default;
+-- 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 IF (NOT) EXISTS
+ALTER TYPE rainbow RENAME VALUE IF EXISTS 'blarm' TO 'fleem';
+ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green' IF NOT 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;
+-- 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
 --
@@ -289,6 +323,7 @@ ROLLBACK;
 --
 DROP TABLE enumtest_child;
 DROP TABLE enumtest_parent;
+DROP TABLE enumtest_default;
 DROP TABLE enumtest;
 DROP TYPE rainbow;

--
2.9.3



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