Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bhase-0003ZR-6g for pgsql-hackers@arkaria.postgresql.org; Wed, 07 Sep 2016 11:19:00 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bhasd-0004DE-JS for pgsql-hackers@arkaria.postgresql.org; Wed, 07 Sep 2016 11:18:59 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1bharF-0002bo-6e for pgsql-hackers@postgresql.org; Wed, 07 Sep 2016 11:17:33 +0000 Received: from ping.uio.no ([2001:700:100:570::196]) by makus.postgresql.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bharB-0002MQ-6f for pgsql-hackers@postgresql.org; Wed, 07 Sep 2016 11:17:31 +0000 Received: from [2001:700:100:570::211] (helo=dalvik.ping.uio.no ident=Debian-exim) by ping.uio.no with esmtp (Exim 4.72 #1 (Debian)) id 1bhar0-0004J6-4H; Wed, 07 Sep 2016 13:17:20 +0200 Received: from ilmari by dalvik.ping.uio.no with local (Exim 4.72) (envelope-from ) id 1bhaqz-0004k0-G3; Wed, 07 Sep 2016 13:17:17 +0200 From: ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) To: emre@hasegeli.com Cc: Tom Lane , Andrew Dunstan , Robert Haas , PostgreSQL Hackers , Matthias Kurz , Jim Nasby , "David G. Johnston" Subject: Re: [PATCH] Alter or rename enum value References: <56E049CC.8010201@dunslane.net> <25605.1457539654@sss.pgh.pa.us> <56E05635.70908@dunslane.net> <28167.1457543612@sss.pgh.pa.us> <26875.1472935684@sss.pgh.pa.us> <32710.1473099022@sss.pgh.pa.us> <8384.1473186627@sss.pgh.pa.us> <9dd58b5f-0447-54fe-02f4-afb62fd7c3c8@dunslane.net> <17893.1473193170@sss.pgh.pa.us> Date: Wed, 07 Sep 2016 12:17:17 +0100 In-Reply-To: (Emre Hasegeli's message of "Wed, 7 Sep 2016 09:44:57 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Exiscan-Spam-Score: -3.7 (---) X-Exiscan-Spam-Report: SpamAssassin 3.3.1 (2010-03-16) on pike.ping.uio.no Score Rule * -5.0 PING_UIO_MAIL_IS_INTERNAL Message has never been outside * 129.240.0.0/16 or 193.157.115.0/24 * 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Pg-Spam-Score: -4.2 (----) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org --=-=-= Emre Hasegeli 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). --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Add-ALTER-TYPE-RENAME-VALUE-TO-v6.patch From e4ee07d53bbab5ee434a12a2f30a9ac9bb5c89d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 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 name RENAME ATTRIBUTE name RENAME TO new_name ALTER TYPE name SET SCHEMA new_schema ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ] +ALTER TYPE name RENAME VALUE existing_enum_value TO new_enum_value where action is one of: @@ -123,6 +124,17 @@ ALTER TYPE name ADD VALUE [ IF NOT + + RENAME VALUE TO + + + 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. + + + + CASCADE @@ -241,7 +253,8 @@ ALTER TYPE name ADD VALUE [ IF NOT new_enum_value - 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. @@ -252,7 +265,8 @@ ALTER TYPE name ADD VALUE [ IF NOT 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. 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 --=-=-= Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers --=-=-=--