From: ilmari@ilmari.org
To: Matthias Kurz <m.kurz@irregular.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Alter or rename enum value
Date: Thu, 24 Mar 2016 18:11:39 +0000
Message-ID: <d8ja8ln7nl0.fsf@dalvik.ping.uio.no> (raw)
In-Reply-To: <CAO=2mx6rK_h4BC7y+3Nu-8fuZJXdFjd9FE_MZnzkH+vnMp0C-Q@mail.gmail.com>
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
[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
--
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-.-ALTER-VALUE-.-TO.patch (text/x-diff, 9.2K) download
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.cindex 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.cindex 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.yindex 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.hindex 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.hindex 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.outindex 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.sqlindex 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
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, m.kurz@irregular.at, tgl@sss.pgh.pa.us, andrew@dunslane.net
Subject: Re: Alter or rename enum value
In-Reply-To: <d8ja8ln7nl0.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