Обсуждение: Alter or rename enum value
<div dir="ltr"><span style="color:rgb(0,0,0);font-size:12.8px">Hi!</span><div style="color:rgb(0,0,0);font-size:12.8px"><br/></div><div style="color:rgb(0,0,0);font-size:12.8px">Right now it is not possibleto rename an enum value.</div><div style="color:rgb(0,0,0);font-size:12.8px">Are there plans to implement this anytimesoon?</div><div style="color:rgb(0,0,0);font-size:12.8px">I had a bit of a discussion on the IRC channel and it seemsit shouldn't be that hard to implement this.</div><div style="color:rgb(0,0,0);font-size:12.8px">Again, I am talkingabout renaming the values, not the enum itself.</div><div style="color:rgb(0,0,0);font-size:12.8px"><br /></div><divstyle="color:rgb(0,0,0);font-size:12.8px">Thanks!</div><div style="color:rgb(0,0,0);font-size:12.8px"><br /></div><divstyle="color:rgb(0,0,0);font-size:12.8px">Greetings,</div><div style="color:rgb(0,0,0);font-size:12.8px">Matthias</div></div>
On 03/09/2016 09:56 AM, Matthias Kurz wrote: > Hi! > > Right now it is not possible to rename an enum value. > Are there plans to implement this anytime soon? > I had a bit of a discussion on the IRC channel and it seems it > shouldn't be that hard to implement this. > Again, I am talking about renaming the values, not the enum itself. > > I don't know of any plans, but it would be a useful thing. I agree it wouldn't be too hard. The workaround is to do an update on pg_enum directly, but proper SQL support would be much nicer. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 03/09/2016 09:56 AM, Matthias Kurz wrote:
>> Right now it is not possible to rename an enum value.
>> Are there plans to implement this anytime soon?
> I don't know of any plans, but it would be a useful thing. I agree it
> wouldn't be too hard. The workaround is to do an update on pg_enum
> directly, but proper SQL support would be much nicer.
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.
regards, tom lane
On 03/09/2016 11:07 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 03/09/2016 09:56 AM, Matthias Kurz wrote: >>> Right now it is not possible to rename an enum value. >>> Are there plans to implement this anytime soon? >> I don't know of any plans, but it would be a useful thing. I agree it >> wouldn't be too hard. The workaround is to do an update on pg_enum >> directly, but proper SQL support would be much nicer. > 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? cheers andrew
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
<div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">Besides not being able to rename enum values there are twoother limitations regarding enums which would be nice to get finally fixed:</div><div class="gmail_extra"><br /></div><divclass="gmail_extra">1) There is also no possibility to drop a value.</div><div class="gmail_extra"><br /></div><divclass="gmail_extra">2) Quoting the docs (<a href="http://www.postgresql.org/docs/9.5/static/sql-altertype.html">http://www.postgresql.org/docs/9.5/static/sql-altertype.html</a>):</div><div class="gmail_extra">"ALTERTYPE ... ADD VALUE (the form that adds a new value to an enum type) cannot be executed inside atransaction block." Example:</div><div class="gmail_extra"># CREATE TYPE bogus AS ENUM('good');</div><div class="gmail_extra">CREATETYPE</div><div class="gmail_extra"># BEGIN;</div><div class="gmail_extra">BEGIN</div><div class="gmail_extra">#ALTER TYPE bogus ADD VALUE 'bad';</div><div class="gmail_extra">ERROR: ALTER TYPE ... ADD cannot runinside a transaction block</div><div class="gmail_extra"><br /></div><div class="gmail_extra">To summarize it:</div><divclass="gmail_extra">For enums to finally be really usable it would nice if we would have (or similiar):</div><divclass="gmail_extra">ALTER TYPE name DROP VALUE [ IF EXISTS ] enum_value</div><div class="gmail_extra">and</div><divclass="gmail_extra">ALTER TYPE name RENAME VALUE [ IF EXISTS ] old_enum_value_name TO new_enum_value_name</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">And all of the operations (adding,renaming, dropping) should also work when done within a new transaction on an enum that existed before that transaction.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">I did some digging and maybe following commitsare useful in this context:</div><div class="gmail_extra">7b90469b71761d240bf5efe3ad5bbd228429278e</div><div class="gmail_extra">c9e2e2db5c2090a880028fd8c1debff474640f50</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Alsothere are these discussions where some of the messages contain some useful information:</div><divclass="gmail_extra"><a href="http://www.postgresql.org/message-id/29F36C7C98AB09499B1A209D48EAA615B7653DBC8A@mail2a.alliedtesting.com">http://www.postgresql.org/message-id/29F36C7C98AB09499B1A209D48EAA615B7653DBC8A@mail2a.alliedtesting.com</a></div><div class="gmail_extra"><a href="http://www.postgresql.org/message-id/50324F26.3090809@dunslane.net">http://www.postgresql.org/message-id/50324F26.3090809@dunslane.net</a></div><div class="gmail_extra"><a href="http://www.postgresql.org/message-id/20130819122938.GB8558@alap2.anarazel.de">http://www.postgresql.org/message-id/20130819122938.GB8558@alap2.anarazel.de</a></div><div class="gmail_extra"><br/></div><div class="gmail_extra">Also have a look at this workaround:</div><div class="gmail_extra"><a href="http://en.dklab.ru/lib/dklab_postgresql_enum/">http://en.dklab.ru/lib/dklab_postgresql_enum/</a></div><div class="gmail_extra"><br/></div><div class="gmail_extra">How high is the chance that given the above information someone willtackle these 3 issues/requests in the near future? It seems there were some internal chances since the introduction ofenums in 8.x so maybe this changes wouldn't be that disruptive anymore?</div><div class="gmail_extra"><br /></div><divclass="gmail_extra">Regards,</div><div class="gmail_extra">Matthias</div><br /><div class="gmail_quote">On 9March 2016 at 18:13, Tom Lane <span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">AndrewDunstan <<a href="mailto:andrew@dunslane.net">andrew@dunslane.net</a>> writes:<br /> > On 03/09/201611:07 AM, Tom Lane wrote:<br /></span><span class="">>> I have a vague recollection that we discussed thisat the time the enum<br /> >> stuff went in, and there are concurrency issues? Don't recall details<br /> >>though.<br /><br /> > Rings a vague bell, but should it be any worse than adding new labels?<br /><br /></span>Ithink what I was recalling is the hazards discussed in the comments for<br /> RenumberEnumType. However, the problemthere is that a backend could make<br /> inconsistent ordering decisions due to seeing two different pg_enum rows<br/> under different snapshots. Updating a single row to change its name<br /> doesn't seem to have a comparable hazard,and it wouldn't affect ordering<br /> anyway. So it's probably no worse than any other object-rename situation.<br/><br /> regards, tom lane<br /></blockquote></div><br /></div></div>
On 9 March 2016 at 20:19, Matthias Kurz <m.kurz@irregular.at> wrote:
Besides not being able to rename enum values there are two other limitations regarding enums which would be nice to get finally fixed:1) There is also no possibility to drop a value.2) Quoting the docs (http://www.postgresql.org/docs/9.5/static/sql-altertype.html):"ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) cannot be executed inside a transaction block." Example:# CREATE TYPE bogus AS ENUM('good');CREATE TYPE# BEGIN;BEGIN# ALTER TYPE bogus ADD VALUE 'bad';ERROR: ALTER TYPE ... ADD cannot run inside a transaction blockTo summarize it:For enums to finally be really usable it would nice if we would have (or similiar):ALTER TYPE name DROP VALUE [ IF EXISTS ] enum_valueandALTER TYPE name RENAME VALUE [ IF EXISTS ] old_enum_value_name TO new_enum_value_nameAnd all of the operations (adding, renaming, dropping) should also work when done within a new transaction on an enum that existed before that transaction.I did some digging and maybe following commits are useful in this context:7b90469b71761d240bf5efe3ad5bbd228429278ec9e2e2db5c2090a880028fd8c1debff474640f50Also there are these discussions where some of the messages contain some useful information:Also have a look at this workaround:How high is the chance that given the above information someone will tackle these 3 issues/requests in the near future? It seems there were some internal chances since the introduction of enums in 8.x so maybe this changes wouldn't be that disruptive anymore?Regards,MatthiasOn 9 March 2016 at 18:13, Tom Lane <tgl@sss.pgh.pa.us> wrote: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)?
Matthias Kurz <m.kurz@irregular.at> writes: [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 >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.c index 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.c index 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.y index 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.h index 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.h index 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.out index 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.sql index 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 -- "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
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 I've added it to the 2016-09 commitfest as well: https://commitfest.postgresql.org/10/588/ -- "A disappointingly low fraction of the human race is,at any given time, on fire." - Stig Sandbeck Mathisen
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
I've added it to the 2016-09 commitfest as well:
https://commitfest.postgresql.org/10/588/
Nice! Thank you!
Actually you still miss a "DROP VALUE" action. Also please make sure this also works when altering an existing enum within a new transaction - otherwise it does not really make sense (Usually someone wants to alter existing enums, not ones that have just been created).
As a result a script like this should pass without problems:
-- ### script start
CREATE TYPE bogus AS ENUM('dog');
-- TEST 1:
BEGIN;
ALTER TYPE bogus ADD VALUE 'cat'; -- fails in 9.5 because of the transaction but should work in future
COMMIT;
-- TEST 2:
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'horse'; -- fails in 9.5 because of the transaction but should work in future
COMMIT;
-- TEST 3:
BEGIN;
ALTER TYPE bogon ALTER VALUE 'dog' TO 'pig'; -- not implemented in 9.5 but should work in future
ROLLBACK;
-- TEST 4:
BEGIN;
ALTER TYPE bogon DROP VALUE 'cat'; -- not implemented in 9.5 but should work in future
ROLLBACK;
-- ### script end
End result of enum "bogon" (which was named "bogus" at the beginning of the script):
-- ###
pig
horse
-- ###
Thank you!
On 3/24/16 2:00 PM, Matthias Kurz wrote: > ALTER TYPE bogon DROP VALUE 'cat'; -- not implemented in 9.5 but should > work in future > ROLLBACK; Dropping a value is significantly harder because that value could be in use. I'm certain there's a really good reason adding new values isn't allowed inside of a transaction. It's probably documented in the code. To answer your question about "what goes into a release", there's really no process for that. What goes into a release is what someone was interested enough in to get community approval for the idea, write the patch, and shepard the patch through the review process. So if you want these features added, you need to either: do it yourself, convince someone else to do it for free, or pay someone to do it for you. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> I'm certain there's a really good reason adding new values isn't allowed
> inside of a transaction. It's probably documented in the code.
Yes, see AlterEnum():
* Ordinarily we disallow adding values within transaction blocks, because * we can't cope with enum OID values
gettinginto indexes and then having * their defining pg_enum entries go away. However, it's okay if the enum *
typewas created in the current transaction, since then there can be no * such indexes that wouldn't themselves go
awayon rollback. (We support * this case because pg_dump --binary-upgrade needs it.)
Deleting an enum value is similarly problematic. Let's assume you're
willing to take out sufficiently widespread locks to prevent entry of
any new rows containing the doomed enum value (which, in reality, is
pretty much unworkable in production situations). Let's assume that
you're willing to hold those locks long enough to VACUUM away every
existing dead row containing that value (see previous parenthetical
comment, squared). You're still screwed, because there might be
instances of the to-be-deleted value sitting in upper levels of btree
indexes (or other index types). There is no mechanism for getting
rid of those, short of a full index rebuild; and you cannot remove
the pg_enum entry without breaking such indexes.
It's conceivable that we could do something like adding an "isdead"
column to pg_enum and making enum_in reject new values that're marked
isdead. But I can't see that we'd ever be able to support true
removal of an enum value at reasonable cost. And I'm not really sure
where the use-case argument is for working hard on it.
regards, tom lane
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">It's conceivablethat we could do something like adding an "isdead"<br /> column to pg_enum and making enum_in reject new valuesthat're marked<br /> isdead. But I can't see that we'd ever be able to support true<br /> removal of an enum valueat reasonable cost. And I'm not really sure<br /> where the use-case argument is for working hard on it.<br /></blockquote></div><br/></div><div class="gmail_extra">Thanks for all your explanation!</div><div class="gmail_extra">Wework a lot with enums and sometimes there are cases where we would like to be able to add a new valueor rename an existing value (in a new transaction) - dropping isn't needed that much, but would be a bonus.</div><divclass="gmail_extra">Right now you have to know which enum values you will use at the time creating a table- but as many things change also requirements for a database/schema/table/enum definition change. At the moment we haveto use ugly hacks to make renaming/dropping work.</div><div class="gmail_extra">I didn't know implementing these featureswould be that complex. As I am not familiar with the code and don't have time to dig into it I won't be able to providea patch myself unfortunately.</div><div class="gmail_extra">Hopefully at the commitfest at least the transaction limitationwill/could be tackled - that would help us a lot already.</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Thanksfor your time!</div></div>
On 03/25/2016 04:13 AM, Matthias Kurz wrote: > > Hopefully at the commitfest at least the transaction limitation > will/could be tackled - that would help us a lot already. > > I don't believe anyone knows how to do that safely. Enums pose special problems here exactly because unlike all other types the set of valid values is mutable. cheers andre
On 3/24/16 10:27 PM, Tom Lane wrote: > It's conceivable that we could do something like adding an "isdead" > column to pg_enum and making enum_in reject new values that're marked > isdead. But I can't see that we'd ever be able to support true > removal of an enum value at reasonable cost. And I'm not really sure > where the use-case argument is for working hard on it. I wonder if we could handle this by allowing foreign keys on enum columns back to pg_enum. Presumably that means we'd have to treat pg_enum as a regular table and not a catalog table. Due to locking concerns I don't think we'd want to put the FKs in place by default either. I've certainly heard people avoiding ENUMs because of their limitations, so it'd be nice if there was a way to lift them. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On 26/03/16 08:17, Jim Nasby wrote: > On 3/24/16 10:27 PM, Tom Lane wrote: >> It's conceivable that we could do something like adding an "isdead" >> column to pg_enum and making enum_in reject new values that're marked >> isdead. But I can't see that we'd ever be able to support true >> removal of an enum value at reasonable cost. And I'm not really sure >> where the use-case argument is for working hard on it. > > I wonder if we could handle this by allowing foreign keys on enum > columns back to pg_enum. Presumably that means we'd have to treat > pg_enum as a regular table and not a catalog table. Due to locking > concerns I don't think we'd want to put the FKs in place by default > either. > > I've certainly heard people avoiding ENUMs because of their > limitations, so it'd be nice if there was a way to lift them. Well, I use Enums extensively in Java. However, I totally avoid using ENUMs in pg, due to their inflexibility! Cheers, Gavin
On Mar 25, 2016, at 11:50 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > I don't believe anyone knows how to do that safely. The core issue, for me, is that not being able to modify enum values in a transaction breaks a very wide variety of databasemigration tools. Even a very brutal solution like marking indexes containing the altered type invalid on a ROLLBACKwould be preferable to the current situation. -- -- Christophe Pettus xof@thebuild.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 03/25/2016 04:13 AM, Matthias Kurz wrote:
>>
>> Hopefully at the commitfest at least the transaction limitation
>> will/could be tackled - that would help us a lot already.
>
> I don't believe anyone knows how to do that safely. Enums pose special
> problems here exactly because unlike all other types the set of valid
> values is mutable.
However, this problem (and the one described in the comments of
AlterEnum()) doesn't apply to altering the name, since that doesn't
affect the OID or the ordering. Attached is version 2 of the patch,
which allows ALTER TYPE ... ALTER VALUE inside a transaction.
It still needs documentation, and possibly support for IF (NOT) EXISTS,
if people think that's useful.
>From 0a482f6d7434964ce51f66c260bde4a74dac4da0 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 '...'
Unlike adding values, altering a value can be done in a transaction,
since it doesn't affect the OID values or ordering, so is safe to
rollback.
TODO: documentation, IF (NOT) EXISTS support(?)
---
src/backend/catalog/pg_enum.c | 88 ++++++++++++++++++++++++++++++++++++++
src/backend/commands/typecmds.c | 50 ++++++++++++----------
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 ++++++++++++
7 files changed, 218 insertions(+), 22 deletions(-)
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..1079e6c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,94 @@ 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 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 227d382..63f030b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1236,32 +1236,38 @@ 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);
+ 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 1273352..b7fea7a 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.h
index 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.h
index 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.out
index 1a61a5b..39bf239 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 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 'crimson';
+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 that renaming a type in a transaction works
+BEGIN;
+ALTER TYPE rainbow ALTER 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 ALTER 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..0863df8 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 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 'crimson';
+-- check that renaming to an existent value fails
+ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green';
+-- check that renaming a type in a transaction works
+BEGIN;
+ALTER TYPE rainbow ALTER 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 ALTER 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.8.0.rc3
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
On 3/25/16 2:22 PM, Gavin Flower wrote: >> >> I've certainly heard people avoiding ENUMs because of their >> limitations, so it'd be nice if there was a way to lift them. > Well, I use Enums extensively in Java. > > However, I totally avoid using ENUMs in pg, due to their inflexibility! Possibly related to this, for a long time I've also wanted a way to better integrate FKs, probably via some kind of a pseudotype or maybe a special operator. The idea being that instead of manually specifying joins, you could treat a FK field in a table as a pointer and do things like: CREATE TABLE invoice(customer int NOT NULL REFERENCES(customer)); SELECT invoice.*, customer->first_name, customer->last_name, ... FROM invoice; If we had that capability, there would be less need for ENUMs. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On 03/25/2016 03:22 PM, Christophe Pettus wrote: > On Mar 25, 2016, at 11:50 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > >> I don't believe anyone knows how to do that safely. > The core issue, for me, is that not being able to modify enum values in a transaction breaks a very wide variety of databasemigration tools. Even a very brutal solution like marking indexes containing the altered type invalid on a ROLLBACKwould be preferable to the current situation. > Well, let's see if we can think harder about when it will be safe to add new enum values and how we can better handle unsafe situations. The danger AIUI is that the new value value will get into an upper level page of an index, and that we can't roll that back if the transaction aborts. First, if there isn't an existing index using the type we should be safe - an index created in the current transaction is not a problem because if the transaction aborts the whole index will disappear. Even if there is an existing index, if it isn't touched by the current transaction the we should still be safe. However, if it is touched we could end up with a corrupted index if the transaction aborts. Maybe we could provide an option to reindex those indexes if they have been touched in the transaction and it aborts. And if it's not present we could instead abort the transaction as soon as it detects something that touches the index. I quite understand that this is all hand-wavy, but I'm trying to get a discussion started that goes beyond acknowledging the pain that the current situation involves. cheers andrew
On Friday, March 25, 2016, Andrew Dunstan <andrew@dunslane.net> wrote:
In some ways it would be a specialized composite type, and we could leverage that to you all the syntax available for those - but without having a different function for each differently named enum classed table since they all would share a common structure, differing only in name. But the tables would be in user space and not a preordained relation in pg_catalog. Maybe require they all inherit from some template but empty table...
On 03/25/2016 04:13 AM, Matthias Kurz wrote:
Hopefully at the commitfest at least the transaction limitation will/could be tackled - that would help us a lot already.
I don't believe anyone knows how to do that safely. Enums pose special problems here exactly because unlike all other types the set of valid values is mutable.
Yeah, I'm not sure there is much blue sky here as long as the definition of an enum is considered system data. It probably needs to be altered so that a user can create a table of class enum with a known layout that PostgreSQL can rely upon to perform optimizations and provide useful behaviors - at least internally. The most visible behavior being displaying the label while ordering using its position.
The system, seeing a data type of that class, would have an implicit reference between columns of that type and the source table.
You have to use normal cascade update/delete/do-nothing while performing DML on the source table.
David J.
On 03/26/2016 12:35 AM, David G. Johnston wrote: > On Friday, March 25, 2016, Andrew Dunstan <andrew@dunslane.net > <mailto:andrew@dunslane.net>> wrote: > > > On 03/25/2016 04:13 AM, Matthias Kurz wrote: > > > Hopefully at the commitfest at least the transaction > limitation will/could be tackled - that would help us a lot > already. > > > I don't believe anyone knows how to do that safely. Enums pose > special problems here exactly because unlike all other types the > set of valid values is mutable. > > > Yeah, I'm not sure there is much blue sky here as long as the > definition of an enum is considered system data. It probably needs to > be altered so that a user can create a table of class enum with a > known layout that PostgreSQL can rely upon to perform optimizations > and provide useful behaviors - at least internally. The most visible > behavior being displaying the label while ordering using its position. > > The system, seeing a data type of that class, would have an implicit > reference between columns of that type and the source table. > You have to use normal cascade update/delete/do-nothing while > performing DML on the source table. > > In some ways it would be a specialized composite type, and we could > leverage that to you all the syntax available for those - but without > having a different function for each differently named enum classed > table since they all would share a common structure, differing only in > name. But the tables would be in user space and not a preordained > relation in pg_catalog. Maybe require they all inherit from some > template but empty table... > > We don't have the luxury of being able to redesign this as a green fields development. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> We don't have the luxury of being able to redesign this as a green
> fields development.
I'm not actually convinced that we need to do anything. SQL already has a
perfectly good mechanism for enforcing that a column contains only values
of a mutable set defined in another table --- it's called a foreign key.
The point of inventing enums was to provide a lower-overhead solution
for cases where the allowed value set is *not* mutable. So okay, if we
can allow certain cases of changing the value set without increasing
the overhead, great. But when we can't do it without adding a whole
lot of complexity and overhead (and, no doubt, bugs), we need to just
say no.
Maybe the docs about enums need to be a little more explicit about
pointing out this tradeoff.
regards, tom lane
On 03/26/2016 10:25 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> We don't have the luxury of being able to redesign this as a green >> fields development. > I'm not actually convinced that we need to do anything. SQL already has a > perfectly good mechanism for enforcing that a column contains only values > of a mutable set defined in another table --- it's called a foreign key. > The point of inventing enums was to provide a lower-overhead solution > for cases where the allowed value set is *not* mutable. So okay, if we > can allow certain cases of changing the value set without increasing > the overhead, great. But when we can't do it without adding a whole > lot of complexity and overhead (and, no doubt, bugs), we need to just > say no. > > Maybe the docs about enums need to be a little more explicit about > pointing out this tradeoff. > > OK, but we (in fact, you and I, for the most part) have provided a way to add new values. The trouble people have is the transaction restriction on that facility, because all the other changes they make with migration tools can be nicely wrapped in a transaction. And the thing is, in my recent experience on a project using quite a few enums, none of the transactions I'd like to include these mutations of enums in does anything that would be at all dangerous. It would be nice if we could find a less broad brush approach to dealing with the issue. cheers andrew
On Mar 26, 2016, at 7:40 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > It would be nice if we could find a less broad brush approach to dealing with the issue. I don't know how doable this is, but could we use the existing mechanism of marking an index invalid if it contains an enumtype to which a value was added, and the transaction was rolled back? For the 90% use case, that would be acceptable,I would expect. -- -- Christophe Pettus xof@thebuild.com
On 03/27/2016 12:43 AM, Christophe Pettus wrote: > On Mar 26, 2016, at 7:40 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >> It would be nice if we could find a less broad brush approach to dealing with the issue. > I don't know how doable this is, but could we use the existing mechanism of marking an index invalid if it contains anenum type to which a value was added, and the transaction was rolled back? For the 90% use case, that would be acceptable,I would expect. > The more I think about this the more I bump up against the fact that almost anything we do might want to do to ameliorate the situation is going to be rolled back. The only approach I can think of that doesn't suffer from this is to abort if an insert/update will affect an index on a modified enum. i.e. we prevent the possible corruption from happening in the first place, as we do now, but in a much more fine grained way. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> The more I think about this the more I bump up against the fact that
> almost anything we do might want to do to ameliorate the situation is
> going to be rolled back. The only approach I can think of that doesn't
> suffer from this is to abort if an insert/update will affect an index on
> a modified enum. i.e. we prevent the possible corruption from happening
> in the first place, as we do now, but in a much more fine grained way.
Perhaps, instead of forbidding ALTER ENUM ADD in a transaction, we could
allow that, but not allow the new value to be *used* until it's committed?
This could be checked cheaply during enum value lookup (ie, is xmin of the
pg_enum row committed).
What you really need is to prevent the new value from being inserted
into any indexes, but checking that directly seems far more difficult,
ugly, and expensive than the above.
I do not know whether this would be a meaningful improvement for
common use-cases, though. (It'd help if people were more specific
about the use-cases they need to work.)
regards, tom lane
On Mar 27, 2016, at 7:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I do not know whether this would be a meaningful improvement for > common use-cases, though. It would certainly be a step forward over the current situation. It would mean that a specific imaginable use-case (insertinga new enum value, then populating a dimension table for it) would have to be done as two migrations rather thanone, but that is much more doable in most tools than having a migration run without a transaction at all. -- -- Christophe Pettus xof@thebuild.com
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) >From 0fee3bdc2e1f4022344e050969b993c963889f55 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=20ALTER=20VALUE?= =?UTF-8?q?=20=E2=80=A6=20TO=20=E2=80=A6=20for=20enums?= 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 | 51 +++++++++------- 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, 284 insertions(+), 24 deletions(-) diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml index 9789881..f6b4e66 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> ALTER 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>ALTER VALUE [ IF EXISTST ] 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 is <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 alreeady 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..2e78294 100644 --- a/src/backend/catalog/pg_enum.c +++ b/src/backend/catalog/pg_enum.c @@ -464,6 +464,123 @@ 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, + 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 227d382..311a1f7 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -1236,32 +1236,39 @@ 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 1273352..e75189e 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -5253,9 +5253,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; } @@ -5263,9 +5265,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; } @@ -5273,12 +5277,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 ALTER 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 8b958b4..3680ce8 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2693,9 +2693,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..33b4e6d 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 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 'crimson'; +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 IF (NOT) EXISTS +ALTER TYPE rainbow ALTER VALUE IF EXISTS 'blarm' TO 'fleem'; +NOTICE: "blarm" is not an existing enum label, skipping +ALTER TYPE rainbow ALTER 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 ALTER 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 ALTER 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..a763220 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 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 'crimson'; +-- check that renaming to an existent value fails +ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green'; +-- check IF (NOT) EXISTS +ALTER TYPE rainbow ALTER VALUE IF EXISTS 'blarm' TO 'fleem'; +ALTER TYPE rainbow ALTER VALUE 'blue' TO 'green' IF NOT EXISTS; +-- check that renaming a value in a transaction works +BEGIN; +ALTER TYPE rainbow ALTER 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 ALTER 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.8.0.rc3 -- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen
On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote: > 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. A couple of trivial comments below. + <term><literal>ALTER VALUE [ IF EXISTST ] TO [ IF NOT EXISTS ]</literal></term> typo EXISTST + If <literal>IF EXISTS</literal> or is <literal>IF NOT EXISTS</literal> + is specified, it is not an error if the type doesn't contain the old double is + if the old value is not alreeady present or the new value is. typo alreeady + * RenameEnumLabel + * Add a new label to the enum set. By default it goes at copypaste-o? + if (!stmt->oldVal) { not project curly brace style .m
Marko Tiikkaja <marko@joh.to> writes: > On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote: >> 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. > > A couple of trivial comments below. Thanks, all fixed locally and will be in the next version of the patch. -- "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 withthe consequences of." --Skud's Meta-Law
> I do not know whether this would be a meaningful improvement for > common use-cases, though. (It'd help if people were more specific > about the use-cases they need to work.) For what its worth, in the company I am working for, InnoGames GmbH, not being able to alter enums is the number one pain point with PostgreSQL. We have hundreds of small databases on the backend of the game worlds. They are heavily using enums. New values to the enums need to be added or to be removed for the new features. We are relying on the transactions for the database migrations, so we couldn't make use of ALTER TYPE ADD VALUE. Some functions found on the Internet [1] which change the values on the catalog had been used, until they corrupted some indexes. (I believe those functions are still quite common.) Now, we are using a function to replace an enum type on all tables with another one, but we are not at all happy with this solution. It requires all objects which were using the enum to be dropped and recreated, and it rewrites the tables, so it greatly increases the migration time and effort. [1] http://en.dklab.ru/lib/dklab_postgresql_enum/
On 3/28/16 4:42 AM, Emre Hasegeli wrote: > Now, we are using a > function to replace an enum type on all tables with another one, but > we are not at all happy with this solution. It requires all objects > which were using the enum to be dropped and recreated, and it rewrites > the tables, so it greatly increases the migration time and effort. FWIW, there are ways to avoid some of that pain by having a trigger maintain the new column on INSERT/UPDATE and then slowly touching all the old rows where the new column is NULL. Obviously would be much better if we could just do this with ENUMs... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On 03/27/2016 10:20 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> The more I think about this the more I bump up against the fact that >> almost anything we do might want to do to ameliorate the situation is >> going to be rolled back. The only approach I can think of that doesn't >> suffer from this is to abort if an insert/update will affect an index on >> a modified enum. i.e. we prevent the possible corruption from happening >> in the first place, as we do now, but in a much more fine grained way. > Perhaps, instead of forbidding ALTER ENUM ADD in a transaction, we could > allow that, but not allow the new value to be *used* until it's committed? > This could be checked cheaply during enum value lookup (ie, is xmin of the > pg_enum row committed). > > What you really need is to prevent the new value from being inserted > into any indexes, but checking that directly seems far more difficult, > ugly, and expensive than the above. > > I do not know whether this would be a meaningful improvement for > common use-cases, though. (It'd help if people were more specific > about the use-cases they need to work.) > > I think this is a pretty promising approach, certainly well worth putting some resources into investigating. One thing I like about it is that it gives a nice cheap negative test, so we know if the xmin is committed we are safe. So we could start by rejecting anything where it's not, but later might adopt a more refined but expensive tests for cases where it isn't committed without imposing a penalty on anything else. cheers andrew
On 03/29/2016 04:56 PM, Andrew Dunstan wrote: > > > On 03/27/2016 10:20 AM, Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >>> The more I think about this the more I bump up against the fact that >>> almost anything we do might want to do to ameliorate the situation is >>> going to be rolled back. The only approach I can think of that doesn't >>> suffer from this is to abort if an insert/update will affect an >>> index on >>> a modified enum. i.e. we prevent the possible corruption from happening >>> in the first place, as we do now, but in a much more fine grained way. >> Perhaps, instead of forbidding ALTER ENUM ADD in a transaction, we could >> allow that, but not allow the new value to be *used* until it's >> committed? >> This could be checked cheaply during enum value lookup (ie, is xmin >> of the >> pg_enum row committed). >> >> What you really need is to prevent the new value from being inserted >> into any indexes, but checking that directly seems far more difficult, >> ugly, and expensive than the above. >> >> I do not know whether this would be a meaningful improvement for >> common use-cases, though. (It'd help if people were more specific >> about the use-cases they need to work.) >> >> > > > I think this is a pretty promising approach, certainly well worth > putting some resources into investigating. One thing I like about it > is that it gives a nice cheap negative test, so we know if the xmin is > committed we are safe. So we could start by rejecting anything where > it's not, but later might adopt a more refined but expensive tests for > cases where it isn't committed without imposing a penalty on anything > else. > > Looking at this briefly. It looks like the check should be called from enum_in() and enum_recv(). What error should be raised if the enum row's xmin isn't committed? ERRCODE_FEATURE_NOT_SUPPORTED? or maybe ERRCODE_DATA_EXCEPTION? I don't see anything that fits very well. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Looking at this briefly. It looks like the check should be called from
> enum_in() and enum_recv(). What error should be raised if the enum row's
> xmin isn't committed? ERRCODE_FEATURE_NOT_SUPPORTED? or maybe
> ERRCODE_DATA_EXCEPTION? I don't see anything that fits very well.
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE is something we use in some
other places where the meaning is "just wait awhile, dude". Or you
could invent a new ERRCODE.
regards, tom lane
Just stumbled across this thread while looking for something else… > On 28 Mar 2016, at 12:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > What you really need is to prevent the new value from being inserted > into any indexes, but checking that directly seems far more difficult, > ugly, and expensive than the above. > > I do not know whether this would be a meaningful improvement for > common use-cases, though. (It'd help if people were more specific > about the use-cases they need to work.) My team’s use case is: We have to add new values to an enum (no removal or renames) during occasional database schema migrationas part of software releases. We’re using a db migration tool that understands that postgres can do most schemachanges in a transaction, so it attempts to run our add enum value statements in a transaction, even if we mark themas individual changes. With some huffing and puffing we’ve managed to work around this limitation in the tool, but itwould definitely be nice to keep our enum-related migrations with our other changes and drop the custom non-transactionalmigration code that we’ve had to write. The suggested solution of disallowing any use of the new value during the same transaction that is added in would work finefor us. In the (so far unprecedented) case that we need to use the new value in a migration at the same time, we’d alwayshave the option of splitting the migration up into two transactions. Cheers Tom
On 04/02/2016 01:20 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Looking at this briefly. It looks like the check should be called from >> enum_in() and enum_recv(). What error should be raised if the enum row's >> xmin isn't committed? ERRCODE_FEATURE_NOT_SUPPORTED? or maybe >> ERRCODE_DATA_EXCEPTION? I don't see anything that fits very well. > ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE is something we use in some > other places where the meaning is "just wait awhile, dude". Or you > could invent a new ERRCODE. > > OK, did that. Here is a patch that is undocumented but I think is otherwise complete. It's been tested a bit and we haven't been able to break it. Comments welcome. cheers andrew
Вложения
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
> 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? I confused with Andrew's transactional enum addition patch. I guess we need a separate entry on Commitfest App for that part of the thread [1]. I am not sure, if that is possible. I will do my best to review both of them. [1] https://www.postgresql.org/message-id/56FFE757.1090301@dunslane.net
> 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. > + int nbr_index; > + Form_pg_enum nbr_en; What is "nbr"? Why not calling them something like "i" and "val"? > + /* 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. > + } > + 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().
Emre Hasegeli <emre@hasegeli.com> writes:
>> + 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().
Transaction abort will clean up such resources just fine; if it did
not, then any function you call would have problems if it threw an
error. I would not contort the logic to free stuff before ereport.
regards, tom lane
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
> 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. Yes, you are right that this is not an identifier like others. On the other hand, all RENAME xxx TO yyy statements are RenameStmt at the moment. I think we better leave the decision to the committer. >> 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. I see. Judging from there, it should be shortcut for neighbour. We better use something else. >> 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. I still don't see any reason not to ReleaseCatCacheList() after the loop instead of writing it 3 times. > Here is an updated patch. I don't know, if it is used by the committer or not, but "existance" is a typo on the commit message. Other than these, it looks good to me. I am marking it as Ready for Committer.
Emre Hasegeli <emre@hasegeli.com> writes:
> Other than these, it looks good to me. I am marking it as Ready for Committer.
I started looking at this patch. I'm kind of unhappy with having *both*
IF EXISTS and IF NOT EXISTS options on the statement, especially since
the locations of those phrases in the syntax seem to have been chosen
with a dartboard. This feels way more confusing than it is useful.
Is there really a strong use-case for either option? I note that
ALTER TABLE RENAME COLUMN, which is probably used a thousand times
more often than this will be, has so far not grown either kind of option,
which sure makes me think that this proposal is getting ahead of itself.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, did that. Here is a patch that is undocumented but I think is
> otherwise complete. It's been tested a bit and we haven't been able to
> break it. Comments welcome.
Got around to looking at this. Attached is a revised version that I think
is in committable shape. The main non-cosmetic change is that the test
for "type was created in same transaction as new value" now consists of
comparing the xmins of the pg_type and pg_enum rows, without consulting
GetCurrentTransactionId(). I did not like the original coding because
it would pointlessly disallow references to enum values that were created
in a parent transaction of the current subxact. This way is still leaving
some subxact use-cases on the table, as noted in the code comments, but
it's more flexible than before.
Barring objections I'll push this soon.
regards, tom lane
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..aec73f6 100644
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
*************** ALTER TYPE <replaceable class="PARAMETER
*** 266,273 ****
<title>Notes</title>
<para>
! <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
! enum type) cannot be executed inside a transaction block.
</para>
<para>
--- 266,275 ----
<title>Notes</title>
<para>
! If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
! an enum type) is executed inside a transaction block, the new value cannot
! be used until after the transaction has been committed, except in the case
! that the enum type itself was created earlier in the same transaction.
</para>
<para>
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index ce04211..8e7be78 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** DefineEnum(CreateEnumStmt *stmt)
*** 1221,1227 ****
* Adds a new label to an existing enum.
*/
ObjectAddress
! AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
{
Oid enum_type_oid;
TypeName *typename;
--- 1221,1227 ----
* Adds a new label to an existing enum.
*/
ObjectAddress
! AlterEnum(AlterEnumStmt *stmt)
{
Oid enum_type_oid;
TypeName *typename;
*************** AlterEnum(AlterEnumStmt *stmt, bool isTo
*** 1236,1260 ****
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");
-
/* Check it's an enum and check user has permission to ALTER the enum */
checkEnumOwner(tup);
--- 1236,1241 ----
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ac50c2a..ac64135 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** ProcessUtilitySlow(Node *parsetree,
*** 1359,1365 ****
break;
case T_AlterEnumStmt: /* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
break;
case T_ViewStmt: /* CREATE VIEW */
--- 1359,1365 ----
break;
case T_AlterEnumStmt: /* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree);
break;
case T_ViewStmt: /* CREATE VIEW */
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 135a544..47d5355 100644
*** a/src/backend/utils/adt/enum.c
--- b/src/backend/utils/adt/enum.c
***************
*** 19,24 ****
--- 19,25 ----
#include "catalog/indexing.h"
#include "catalog/pg_enum.h"
#include "libpq/pqformat.h"
+ #include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
*************** static Oid enum_endpoint(Oid enumtypoid,
*** 31,36 ****
--- 32,124 ----
static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
+ /*
+ * Disallow use of an uncommitted pg_enum tuple.
+ *
+ * We need to make sure that uncommitted enum values don't get into indexes.
+ * If they did, and if we then rolled back the pg_enum addition, we'd have
+ * broken the index because value comparisons will not work reliably without
+ * an underlying pg_enum entry. (Note that removal of the heap entry
+ * containing an enum value is not sufficient to ensure that it doesn't appear
+ * in upper levels of indexes.) To do this we prevent an uncommitted row from
+ * being used for any SQL-level purpose. This is stronger than necessary,
+ * since the value might not be getting inserted into a table or there might
+ * be no index on its column, but it's easy to enforce centrally.
+ *
+ * However, it's okay to allow use of uncommitted values belonging to enum
+ * types that were themselves created in the same transaction, because then
+ * any such index would also be new and would go away altogether on rollback.
+ * (This case is required by pg_upgrade.)
+ *
+ * This function needs to be called (directly or indirectly) in any of the
+ * functions below that could return an enum value to SQL operations.
+ */
+ static void
+ check_safe_enum_use(HeapTuple enumval_tup)
+ {
+ TransactionId xmin;
+ Form_pg_enum en;
+ HeapTuple enumtyp_tup;
+
+ /*
+ * If the row is hinted as committed, it's surely safe. This provides a
+ * fast path for all normal use-cases.
+ */
+ if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
+ return;
+
+ /*
+ * Usually, a row would get hinted as committed when it's read or loaded
+ * into syscache; but just in case not, let's check the xmin directly.
+ */
+ xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
+ if (!TransactionIdIsInProgress(xmin) &&
+ TransactionIdDidCommit(xmin))
+ return;
+
+ /* It is a new enum value, so check to see if the whole enum is new */
+ en = (Form_pg_enum) GETSTRUCT(enumval_tup);
+ enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));
+ if (!HeapTupleIsValid(enumtyp_tup))
+ elog(ERROR, "cache lookup failed for type %u", en->enumtypid);
+
+ /*
+ * We insist that the type have been created in the same (sub)transaction
+ * as the enum value. It would be safe to allow the type's originating
+ * xact to be a subcommitted child of the enum value's xact, but not vice
+ * versa (since we might now be in a subxact of the type's originating
+ * xact, which could roll back along with the enum value's subxact). The
+ * former case seems a sufficiently weird usage pattern as to not be worth
+ * spending code for, so we're left with a simple equality check.
+ *
+ * We also insist that the type's pg_type row not be HEAP_UPDATED. If it
+ * is, we can't tell whether the row was created or only modified in the
+ * apparent originating xact, so it might be older than that xact. (We do
+ * not worry whether the enum value is HEAP_UPDATED; if it is, we might
+ * think it's too new and throw an unnecessary error, but we won't allow
+ * an unsafe case.)
+ */
+ if (xmin == HeapTupleHeaderGetXmin(enumtyp_tup->t_data) &&
+ !(enumtyp_tup->t_data->t_infomask & HEAP_UPDATED))
+ {
+ /* same (sub)transaction, so safe */
+ ReleaseSysCache(enumtyp_tup);
+ return;
+ }
+
+ /*
+ * There might well be other tests we could do here to narrow down the
+ * unsafe conditions, but for now just raise an exception.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE),
+ errmsg("unsafe use of new value \"%s\" of enum type %s",
+ NameStr(en->enumlabel),
+ format_type_be(en->enumtypid)),
+ errhint("New enum values must be committed before they can be used.")));
+ }
+
+
/* Basic I/O support */
Datum
*************** enum_in(PG_FUNCTION_ARGS)
*** 59,64 ****
--- 147,155 ----
format_type_be(enumtypoid),
name)));
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(tup);
+
/*
* This comes from pg_enum.oid and stores system oids in user tables. This
* oid must be preserved by binary upgrades.
*************** enum_recv(PG_FUNCTION_ARGS)
*** 124,129 ****
--- 215,223 ----
format_type_be(enumtypoid),
name)));
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(tup);
+
enumoid = HeapTupleGetOid(tup);
ReleaseSysCache(tup);
*************** enum_endpoint(Oid enumtypoid, ScanDirect
*** 327,335 ****
--- 421,436 ----
enum_tuple = systable_getnext_ordered(enum_scan, direction);
if (HeapTupleIsValid(enum_tuple))
+ {
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(enum_tuple);
minmax = HeapTupleGetOid(enum_tuple);
+ }
else
+ {
+ /* should only happen with an empty enum */
minmax = InvalidOid;
+ }
systable_endscan_ordered(enum_scan);
index_close(enum_idx, AccessShareLock);
*************** enum_range_internal(Oid enumtypoid, Oid
*** 490,495 ****
--- 591,599 ----
if (left_found)
{
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(enum_tuple);
+
if (cnt >= max)
{
max *= 2;
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index be924d5..e7bdb92 100644
*** a/src/backend/utils/errcodes.txt
--- b/src/backend/utils/errcodes.txt
*************** Section: Class 55 - Object Not In Prereq
*** 398,403 ****
--- 398,404 ----
55006 E ERRCODE_OBJECT_IN_USE object_in_use
55P02 E ERRCODE_CANT_CHANGE_RUNTIME_PARAM cant_change_runtime_param
55P03 E ERRCODE_LOCK_NOT_AVAILABLE lock_not_available
+ 55P04 E ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE unsafe_new_enum_value_usage
Section: Class 57 - Operator Intervention
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index e4c86f1..847b770 100644
*** a/src/include/commands/typecmds.h
--- b/src/include/commands/typecmds.h
*************** extern void RemoveTypeById(Oid typeOid);
*** 26,32 ****
extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
extern ObjectAddress DefineRange(CreateRangeStmt *stmt);
! extern ObjectAddress AlterEnum(AlterEnumStmt *stmt, bool isTopLevel);
extern ObjectAddress DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
--- 26,32 ----
extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
extern ObjectAddress DefineRange(CreateRangeStmt *stmt);
! extern ObjectAddress AlterEnum(AlterEnumStmt *stmt);
extern ObjectAddress DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 1a61a5b..d4a45a3 100644
*** a/src/test/regress/expected/enum.out
--- b/src/test/regress/expected/enum.out
*************** DROP TYPE bogus;
*** 560,584 ****
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can't add new values to existing enums in a transaction
BEGIN;
! ALTER TYPE bogus ADD VALUE 'bad';
! ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
COMMIT;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
! ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
ROLLBACK;
DROP TYPE bogus;
! -- check that we *can* add new values to existing enums in a transaction,
! -- if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM();
! ALTER TYPE bogus ADD VALUE 'good';
ALTER TYPE bogus ADD VALUE 'ugly';
ROLLBACK;
--
-- Cleanup
--- 560,631 ----
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can add new values to existing enums in a transaction
! -- but we can't use them
BEGIN;
! ALTER TYPE bogus ADD VALUE 'new';
! SAVEPOINT x;
! SELECT 'new'::bogus; -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! LINE 1: SELECT 'new'::bogus;
! ^
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
! SELECT enum_first(null::bogus); -- safe
! enum_first
! ------------
! good
! (1 row)
!
! SELECT enum_last(null::bogus); -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
! SELECT enum_range(null::bogus); -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
COMMIT;
+ SELECT 'new'::bogus; -- now safe
+ bogus
+ -------
+ new
+ (1 row)
+
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'bogus'::regtype
+ ORDER BY 2;
+ enumlabel | enumsortorder
+ -----------+---------------
+ good | 1
+ new | 2
+ (2 rows)
+
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn; this should not be considered safe
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
! SELECT 'bad'::bogon;
! ERROR: unsafe use of new value "bad" of enum type bogon
! LINE 1: SELECT 'bad'::bogon;
! ^
! HINT: New enum values must be committed before they can be used.
ROLLBACK;
DROP TYPE bogus;
! -- check that we can add new values to existing enums in a transaction
! -- and use them, if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM('good');
! ALTER TYPE bogus ADD VALUE 'bad';
ALTER TYPE bogus ADD VALUE 'ugly';
+ SELECT enum_range(null::bogus);
+ enum_range
+ -----------------
+ {good,bad,ugly}
+ (1 row)
+
ROLLBACK;
--
-- Cleanup
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 88a835e..d25e8de 100644
*** a/src/test/regress/sql/enum.sql
--- b/src/test/regress/sql/enum.sql
*************** DROP TYPE bogus;
*** 262,287 ****
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can't add new values to existing enums in a transaction
BEGIN;
! ALTER TYPE bogus ADD VALUE 'bad';
COMMIT;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
ROLLBACK;
DROP TYPE bogus;
! -- check that we *can* add new values to existing enums in a transaction,
! -- if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM();
! ALTER TYPE bogus ADD VALUE 'good';
ALTER TYPE bogus ADD VALUE 'ugly';
ROLLBACK;
--
--- 262,303 ----
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can add new values to existing enums in a transaction
! -- but we can't use them
BEGIN;
! ALTER TYPE bogus ADD VALUE 'new';
! SAVEPOINT x;
! SELECT 'new'::bogus; -- unsafe
! ROLLBACK TO x;
! SELECT enum_first(null::bogus); -- safe
! SELECT enum_last(null::bogus); -- unsafe
! ROLLBACK TO x;
! SELECT enum_range(null::bogus); -- unsafe
! ROLLBACK TO x;
COMMIT;
+ SELECT 'new'::bogus; -- now safe
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'bogus'::regtype
+ ORDER BY 2;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn; this should not be considered safe
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
+ SELECT 'bad'::bogon;
ROLLBACK;
DROP TYPE bogus;
! -- check that we can add new values to existing enums in a transaction
! -- and use them, if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM('good');
! ALTER TYPE bogus ADD VALUE 'bad';
ALTER TYPE bogus ADD VALUE 'ugly';
+ SELECT enum_range(null::bogus);
ROLLBACK;
--
> Got around to looking at this. Attached is a revised version that I think > is in committable shape. The main non-cosmetic change is that the test > for "type was created in same transaction as new value" now consists of > comparing the xmins of the pg_type and pg_enum rows, without consulting > GetCurrentTransactionId(). I did not like the original coding because > it would pointlessly disallow references to enum values that were created > in a parent transaction of the current subxact. This way is still leaving > some subxact use-cases on the table, as noted in the code comments, but > it's more flexible than before. Thank you for picking this up. The patch looks good to me. I think this is a useful to support adding values to the enum created in the same transaction. > + /* > + * If the row is hinted as committed, it's surely safe. This provides a > + * fast path for all normal use-cases. > + */ > + if (HeapTupleHeaderXminCommitted(enumval_tup->t_data)) > + return; > + > + /* > + * Usually, a row would get hinted as committed when it's read or loaded > + * into syscache; but just in case not, let's check the xmin directly. > + */ > + xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data); > + if (!TransactionIdIsInProgress(xmin) && > + TransactionIdDidCommit(xmin)) > + return; This looks like transaction internal logic inside enum.c to me. Maybe it is because I am not much familiar with the internals. I couldn't find a similar pattern anywhere else, but it might still be useful to invent something like HeapTupleHeaderXminReallyCommitted(). One risk about this feature is that the future enum functions would not check if the value is safe to return. Maybe we should append a warning to the file header about this.
Emre Hasegeli <emre@hasegeli.com> writes:
>> + /*
>> + * If the row is hinted as committed, it's surely safe. This provides a
>> + * fast path for all normal use-cases.
>> + */
>> + if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
>> + return;
>> +
>> + /*
>> + * Usually, a row would get hinted as committed when it's read or loaded
>> + * into syscache; but just in case not, let's check the xmin directly.
>> + */
>> + xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
>> + if (!TransactionIdIsInProgress(xmin) &&
>> + TransactionIdDidCommit(xmin))
>> + return;
> This looks like transaction internal logic inside enum.c to me. Maybe
> it is because I am not much familiar with the internals. I couldn't
> find a similar pattern anywhere else, but it might still be useful to
> invent something like HeapTupleHeaderXminReallyCommitted().
I wondered about that too, but there are no other roughly similar cases
that I could find, and abstracting from a single use-case is perilous ---
especially when there's no compelling reason to think there will ever be
any other similar use-cases. I'd originally wondered whether we could
replace this logic with a call to something in tqual.c, but none of the
available functions there produce the required behavior either.
regards, tom lane
> I started looking at this patch. I'm kind of unhappy with having *both* > IF EXISTS and IF NOT EXISTS options on the statement, especially since > the locations of those phrases in the syntax seem to have been chosen > with a dartboard. This feels way more confusing than it is useful. > Is there really a strong use-case for either option? I note that > ALTER TABLE RENAME COLUMN, which is probably used a thousand times > more often than this will be, has so far not grown either kind of option, > which sure makes me think that this proposal is getting ahead of itself. I think they can be useful. I am writing a lot of migration scripts for small projects. It really helps to be able to run parts of them again. ALTER TYPE ... ADD VALUE already have IF NOT EXISTS option. I don't think we would lose anything to support both of them in here. The syntax ALTER TYPE ... RENAME VALUE [ IF EXISTS ] ... TO ... [ IF NOT EXISTS ] looks self-explaining to me. I haven't confused when I first saw. IF EXISTS applying to the old value, IF NOT EXISTS applying to the new value, are the only reasonable semantics one might expect from renaming things. Anybody is interpreting it wrong? or can think of another syntax?
Emre Hasegeli <emre@hasegeli.com> writes:
>> I started looking at this patch. I'm kind of unhappy with having *both*
>> IF EXISTS and IF NOT EXISTS options on the statement, especially since
>> the locations of those phrases in the syntax seem to have been chosen
>> with a dartboard. This feels way more confusing than it is useful.
>> Is there really a strong use-case for either option? I note that
>> ALTER TABLE RENAME COLUMN, which is probably used a thousand times
>> more often than this will be, has so far not grown either kind of option,
>> which sure makes me think that this proposal is getting ahead of itself.
> I think they can be useful. I am writing a lot of migration scripts
> for small projects. It really helps to be able to run parts of them
> again. ALTER TYPE ... ADD VALUE already have IF NOT EXISTS option. I
> don't think we would lose anything to support both of them in here.
The opportunity cost here is potential user confusion. The only
closely parallel rename operation we have is ALTER TABLE RENAME COLUMN,
and that doesn't have a column-level IF EXISTS option; it has a
table-level IF EXISTS option. So I think it would be weird and confusing
for ALTER TYPE RENAME VALUE to be different from that. And again, it's
hard to get excited about having these options for RENAME VALUE when no
one has felt a need for them yet in RENAME COLUMN. I'm especially dubious
about IF NOT EXISTS against the destination name, considering that there
isn't *any* variant of RENAME that has an equivalent of that. If it's
really useful, why hasn't that happened?
I think we should leave these features out for now and wait to see if
there's really field demand for them. If there is, it probably will
make sense to add them in more than just this one kind of RENAME.
regards, tom lane
On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The opportunity cost here is potential user confusion. The only > closely parallel rename operation we have is ALTER TABLE RENAME COLUMN, > and that doesn't have a column-level IF EXISTS option; it has a > table-level IF EXISTS option. So I think it would be weird and confusing > for ALTER TYPE RENAME VALUE to be different from that. And again, it's > hard to get excited about having these options for RENAME VALUE when no > one has felt a need for them yet in RENAME COLUMN. I'm especially dubious > about IF NOT EXISTS against the destination name, considering that there > isn't *any* variant of RENAME that has an equivalent of that. If it's > really useful, why hasn't that happened? Because Tom Lane keeps voting against every patch to expand IF [ NOT ] EXISTS into a new area? :-) We do have ALTER TABLE [ IF EXISTS ] .. ADD COLUMN [ IF NOT EXISTS ], so if somebody wanted the [ IF NOT EXISTS ] clause to also apply to the RENAME COLUMN case, they'd have a good argument for adding it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... And again, it's
>> hard to get excited about having these options for RENAME VALUE when no
>> one has felt a need for them yet in RENAME COLUMN. I'm especially dubious
>> about IF NOT EXISTS against the destination name, considering that there
>> isn't *any* variant of RENAME that has an equivalent of that. If it's
>> really useful, why hasn't that happened?
> Because Tom Lane keeps voting against every patch to expand IF [ NOT ]
> EXISTS into a new area? :-)
Well, I'm on record as not liking the squishy semantics of CREATE IF NOT
EXISTS, and you could certainly make the same argument against RENAME IF
NOT EXISTS: you don't really know what state you will have after the
command executes. But that wasn't the point I was trying to make here.
> We do have ALTER TABLE [ IF EXISTS ] .. ADD COLUMN [ IF NOT EXISTS ],
> so if somebody wanted the [ IF NOT EXISTS ] clause to also apply to
> the RENAME COLUMN case, they'd have a good argument for adding it.
If someone wanted to propose adding IF NOT EXISTS to our rename
commands across-the-board, that would be a sensible feature to discuss.
What I'm objecting to is this one niche-case command getting out in
front of far-more-widely-used commands in terms of having such features.
I think the fact that we don't already have it in other rename commands
is pretty strong evidence that this is a made-up feature rather than
something with actual field demand. I'm also concerned about adding it
in just one place like this; we might find ourselves boxed in in terms of
hitting syntax conflicts when we try to duplicate the feature elsewhere,
if we haven't done the legwork to add it to all variants of RENAME at
the same time.
regards, tom lane
On 09/06/2016 02:30 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> ... And again, it's >>> hard to get excited about having these options for RENAME VALUE when no >>> one has felt a need for them yet in RENAME COLUMN. I'm especially dubious >>> about IF NOT EXISTS against the destination name, considering that there >>> isn't *any* variant of RENAME that has an equivalent of that. If it's >>> really useful, why hasn't that happened? >> Because Tom Lane keeps voting against every patch to expand IF [ NOT ] >> EXISTS into a new area? :-) > Well, I'm on record as not liking the squishy semantics of CREATE IF NOT > EXISTS, and you could certainly make the same argument against RENAME IF > NOT EXISTS: you don't really know what state you will have after the > command executes. But that wasn't the point I was trying to make here. > >> We do have ALTER TABLE [ IF EXISTS ] .. ADD COLUMN [ IF NOT EXISTS ], >> so if somebody wanted the [ IF NOT EXISTS ] clause to also apply to >> the RENAME COLUMN case, they'd have a good argument for adding it. > If someone wanted to propose adding IF NOT EXISTS to our rename > commands across-the-board, that would be a sensible feature to discuss. > What I'm objecting to is this one niche-case command getting out in > front of far-more-widely-used commands in terms of having such features. > I think the fact that we don't already have it in other rename commands > is pretty strong evidence that this is a made-up feature rather than > something with actual field demand. I'm also concerned about adding it > in just one place like this; we might find ourselves boxed in in terms of > hitting syntax conflicts when we try to duplicate the feature elsewhere, > if we haven't done the legwork to add it to all variants of RENAME at > the same time. > > Are we also going to have an exists test for the original thing being renamed? Exists tests on renames do strike me as somewhat cumbersome, to say the least. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Are we also going to have an exists test for the original thing being
> renamed? Exists tests on renames do strike me as somewhat cumbersome, to
> say the least.
I'm less opposed to that part, because it's at least got *some* precedent
in existing RENAME features. But the fact that RENAME COLUMN hasn't got
it still makes me wonder why this is the first place that's getting it
("it" being an EXISTS test that applies to a sub-object rather than the
whole object being ALTER'd).
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.
regards, tom lane
> 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.
Emre Hasegeli <emre@hasegeli.com> 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).
>From e4ee07d53bbab5ee434a12a2f30a9ac9bb5c89d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
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 <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 <replaceable
class="PARAMETER">existing_enum_value</replaceable>TO <replaceable class="PARAMETER">new_enum_value</replaceable>
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
@@ -123,6 +124,17 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>RENAME VALUE TO</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.
+ 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 +253,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 +265,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 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
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
> 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).
Pushed with some adjustments. The only thing that wasn't a matter of
taste is you forgot to update the backend/nodes/ support functions
for struct AlterEnumStmt.
regards, tom lane
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 7 September 2016 at 22:14, Tom Lane <span dir="ltr"><<ahref="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span> wrote:<br /><blockquoteclass="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="gmail-"><ahref="mailto:ilmari@ilmari.org">ilmari@ilmari.org</a> (Dagfinn Ilmari Mannsåker) writes:<br /> > Hereis version 6 of the patch, which just adds RENAME VALUE with no IF<br /> > [NOT] EXISTS, rebased onto current master(particularly the<br /> > transactional ADD VALUE patch).<br /><br /></span>Pushed with some adjustments. The onlything that wasn't a matter of<br /> taste is you forgot to update the backend/nodes/ support functions<br /> for structAlterEnumStmt.<br /><br /> regards, tom lane<br /></blockquote></div><br /></div><div class="gmail_extra">Thankyou all for committing this feature!</div><div class="gmail_extra"><br /></div><div class="gmail_extra">Giventhat you are now familiar with the internals of how enums are implemented would it be possible tocontinue the work and add the "ALTER TYPE <name> DROP VALUE <somevalue>" command?</div><div class="gmail_extra"><br/></div><div class="gmail_extra">Thank you!</div><div class="gmail_extra">Regards, Matthias</div></div>
> Given that you are now familiar with the internals of how enums are > implemented would it be possible to continue the work and add the "ALTER > TYPE <name> DROP VALUE <somevalue>" command? I was thinking to try developing it, but I would be more than happy to help by testing and reviewing, if someone else would do. I don't think I have enough experience to think of all details of this feature. The main problem that has been discussed before was the indexes. One way is to tackle with it is to reindex all the tables after the operation. Currently we are doing it when the datatype of indexed columns change. So it should be possible, but very expensive. Another way, Thomas Munro suggested when we were talking about it, would be to add another column to mark the deleted rows to the catalog table. We can check for this column before allowing the value to be used. Indexes can continue using the deleted values. We can also re-use those entries when someone wants to add new value to the matching place. It should be safe as long as we don't update the sort order.
On 9/8/16 4:55 AM, Emre Hasegeli wrote: > The main problem that has been discussed before was the indexes. One > way is to tackle with it is to reindex all the tables after the > operation. Currently we are doing it when the datatype of indexed > columns change. So it should be possible, but very expensive. Why not just disallow dropping a value that's still in use? That's certainly what I would prefer to happen by default... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
> Why not just disallow dropping a value that's still in use? That's certainly > what I would prefer to happen by default... Of course, we should disallow it. That problem is what to do next. We cannot just remove the value, because it might still be referenced from the inner nodes of the indexes.
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 9/8/16 4:55 AM, Emre Hasegeli wrote:
>> The main problem that has been discussed before was the indexes. One
>> way is to tackle with it is to reindex all the tables after the
>> operation. Currently we are doing it when the datatype of indexed
>> columns change. So it should be possible, but very expensive.
> Why not just disallow dropping a value that's still in use? That's
> certainly what I would prefer to happen by default...
Even ignoring the hidden-values-in-indexes problem, how would you
discover that it's still in use? Not to mention preventing new
insertions after you look?
regards, tom lane