Re: [PATCH] Alter or rename enum value
| От | ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) |
|---|---|
| Тема | Re: [PATCH] Alter or rename enum value |
| Дата | |
| Msg-id | d8jeg5m4as8.fsf@dalvik.ping.uio.no обсуждение исходный текст |
| Ответ на | [PATCH] Alter or rename enum value (ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)) |
| Ответы |
Re: [PATCH] Alter or rename enum value
(Emre Hasegeli <emre@hasegeli.com>)
Re: [PATCH] Alter or rename enum value (Emre Hasegeli <emre@hasegeli.com>) |
| Список | pgsql-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? >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.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> @@ -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.c index 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.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..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.sql index 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 -- - 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
В списке pgsql-hackers по дате отправления:
Следующее
От: Amit KapilaДата:
Сообщение: Re: [PATCH] bigint txids vs 'xid' type, new txid_recent(bigint) => xid