From: ilmari@ilmari.org
To: pgsql-hackers@postgresql.org
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: Matthias Kurz <m.kurz@irregular.at>
Cc: Jim Nasby <Jim.Nasby@BlueTreble.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Emre Hasegeli <emre@hasegeli.com>
Subject: Re: [PATCH] Alter or rename enum value
Date: Thu, 18 Aug 2016 11:43:51 +0100
Message-ID: <d8jeg5m4as8.fsf@dalvik.ping.uio.no> (raw)
In-Reply-To: <d8jk2kn4yn0.fsf_-_@dalvik.ping.uio.no>
References: <CAO=2mx6uvgPaPDf-rHqG8=1MZnGyVDMQeh8zS4euRyyg4D35OQ@mail.gmail.com>
<56E049CC.8010201@dunslane.net>
<25605.1457539654@sss.pgh.pa.us>
<56E05635.70908@dunslane.net>
<28167.1457543612@sss.pgh.pa.us>
<CAO=2mx4rQHRvifT2yUQ1rz90A=9CVz4_Z0Sx1wTgwgQRzAimpQ@mail.gmail.com>
<CAO=2mx6rK_h4BC7y+3Nu-8fuZJXdFjd9FE_MZnzkH+vnMp0C-Q@mail.gmail.com>
<d8ja8ln7nl0.fsf@dalvik.ping.uio.no>
<d8jk2kn4yn0.fsf_-_@dalvik.ping.uio.no>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>
>> 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
>
> Here's v3 of the patch of the patch, which I consider ready for proper
> review. It now features:
>
> - IF (NOT) EXISTS support
> - Transaction support
> - Documentation
> - Improved error reporting (renaming a non-existent value to an existing
> one complains about the former, not the latter)
Here is v4, which changes the command from ALTER VALUE to RENAME VALUE,
for consistency with RENAME ATTRIBUTE.
Emre, I noticed you modified the commitfest entry
(https://commitfest.postgresql.org/10/588/) to be for Andrew's
transactional enum addition patch instead, but didn't change the title.
I'll revert that as soon as it picks up this latest patch. Do you wish
to remain a reviewer for this patch, or should I remove you?
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:0001-Add-ALTER-TYPE-RENAME-VALUE-TO-v4.patch (text/x-diff, 17.0K) download
From 225e3819317aa82fee91afe4970a0596f316cf9f 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 | 115 +++++++++++++++++++++++++++++++++++++
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 | 63 ++++++++++++++++++++
src/test/regress/sql/enum.sql | 30 ++++++++++
8 files changed, 283 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgmlindex 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 NOT EXISTS ]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
@@ -123,6 +124,23 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
</listitem>
</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>
@@ -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.cindex af89daa..5d4c665 100644--- a/src/backend/catalog/pg_enum.c+++ b/src/backend/catalog/pg_enum.c@@ -463,6 +463,121 @@ 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 (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)+ {+ 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.cindex 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.yindex 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.hindex 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.hindex 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.outindex 1a61a5b..62dfc57 100644--- a/src/test/regress/expected/enum.out+++ b/src/test/regress/expected/enum.out@@ -556,6 +556,69 @@ 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 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
--
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sqlindex 88a835e..dc678b8 100644--- a/src/test/regress/sql/enum.sql+++ b/src/test/regress/sql/enum.sql@@ -257,6 +257,36 @@ 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 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
--
--
2.9.3
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilmari@ilmari.org, tgl@sss.pgh.pa.us, andrew@dunslane.net, m.kurz@irregular.at, Jim.Nasby@BlueTreble.com, david.g.johnston@gmail.com, emre@hasegeli.com
Subject: Re: [PATCH] Alter or rename enum value
In-Reply-To: <d8jeg5m4as8.fsf@dalvik.ping.uio.no>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox