Re: [HACKERS] [PATCH] Alter or rename enum value
Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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 DROP VALUE " 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
On 7 September 2016 at 22:14, Tom Lane wrote: > ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) 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 > Thank you all for committing this feature! 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 DROP VALUE " command? Thank you! Regards, Matthias
Re: [HACKERS] [PATCH] Alter or rename enum value
ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Emre Hasegeli writes: >> Bottom line here is that I'd rather commit ALTER TYPE RENAME VALUE with >> no EXISTS features and then see it accrete those features together with >> other types of RENAME, when and if there's a will to make that happen. > > This sounds like a good conclusion to me. Works for me. I mainly added the IF [NOT] EXISTS support to be consistent with ADD VALUE, and because I like idempotency, but I'm not married to it. Here is version 6 of the patch, which just adds RENAME VALUE with no IF [NOT] EXISTS, rebased onto current master (particularly the transactional ADD VALUE patch). >From e4ee07d53bbab5ee434a12a2f30a9ac9bb5c89d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= Date: Tue, 6 Sep 2016 14:38:06 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Add=20ALTER=20TYPE=20=E2=80=A6=20RENAME=20V?= =?UTF-8?q?ALUE=20=E2=80=A6=20TO=20=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unlike adding values to an enum, altering an existing value doesn't affect the OID values or ordering, so can be done in a transaction regardless of whether the enum was created in the same transaction. --- doc/src/sgml/ref/alter_type.sgml | 18 +++- src/backend/catalog/pg_enum.c | 91 ++ src/backend/commands/typecmds.c| 12 +++-- src/backend/parser/gram.y | 14 ++ src/include/catalog/pg_enum.h | 2 + src/include/nodes/parsenodes.h | 1 + src/test/regress/expected/enum.out | 58 src/test/regress/sql/enum.sql | 27 +++ 8 files changed, 217 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml index aec73f6..bdc2fa2 100644 --- a/doc/src/sgml/ref/alter_type.sgml +++ b/doc/src/sgml/ref/alter_type.sgml @@ -29,6 +29,7 @@ ALTER TYPE name RENAME ATTRIBUTE name RENAME TO new_name ALTER TYPE name SET SCHEMA new_schema ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ] +ALTER TYPE name RENAME VALUE existing_enum_value TO new_enum_value where action is one of: @@ -123,6 +124,17 @@ ALTER TYPE name ADD VALUE [ IF NOT + +RENAME VALUE TO + + + This form renames a value in an enum type. + The value's place in the enum's ordering is not affected. + An error will occur if the old value is not already present or the new value is. + + + + CASCADE @@ -241,7 +253,8 @@ ALTER TYPE name ADD VALUE [ IF NOT new_enum_value -The new value to be added to an enum type's list of values. +The new value to be added to an enum type's list of values, +or to rename an existing one to. Like all enum literals, it needs to be quoted. @@ -252,7 +265,8 @@ ALTER TYPE name ADD VALUE [ IF NOT The existing enum value that the new value should be added immediately -before or after in the enum type's sort ordering. +before or after in the enum type's sort ordering, +or renamed from. Like all enum literals, it needs to be quoted. diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index c66f963..2e48dfe 100644 --- a/src/backend/catalog/pg_enum.c +++ b/src/backend/catalog/pg_enum.c @@ -465,6 +465,97 @@ AddEnumLabel(Oid enumTypeOid, } +/* + * RenameEnumLabel + * Rename a label in an enum set. + */ +void +RenameEnumLabel(Oid enumTypeOid, +const char *oldVal, +const char *newVal) +{ + Relation pg_enum; + HeapTuple old_tup = NULL; + HeapTuple enum_tup; + CatCList *list; + int nelems; + int nbr_index; + Form_pg_enum nbr_en; + boolfound_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
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Andrew Dunstan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
On 09/06/2016 02:30 PM, Tom Lane wrote: Robert Haas writes: On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Robert Haas writes: > On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Emre Hasegeli 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Emre Hasegeli 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
Emre Hasegeli 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?= Date: Thu, 24 Mar 2016 17:50:58 + 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 name RENAME ATTRIBUTE name RENAME TO new_name ALTER TYPE name SET SCHEMA new_schema ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ] +ALTER TYPE name RENAME VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ] where action is one of: @@ -124,6 +125,23 @@ ALTER TYPE name ADD VALUE [ IF NOT +RENAME VALUE [ IF EXISTS ] TO [ IF NOT EXISTS ] + + + This form renames a value in an enum type. The value's place in the + enum's ordering is not affected. + + + If IF EXISTS or IF NOT EXISTS 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. + + + + + CASCADE @@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT new_enum_value -The new value to be added to an enum type's list of values. +The new value to be added to an enum type's list of values, +or to rename an existing one to. Like all enum literals, it needs to be quoted. @@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT The existing enum value that the new value should be added immediately -before or after in the enum type's sort ordering. +before or after in the enum type's sort ordering, +or renamed from. Like all enum literals, it needs to be quoted. diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index af89daa..1920138 100644 --- a/src/backend/catalog/pg_enum.c +++ b/src/backend/catalog/pg_enum.c @@
Re: [HACKERS] [PATCH] Alter or rename enum value
Emre Hasegeli 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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(). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
> 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.1090...@dunslane.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > ilm...@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?= Date: Thu, 24 Mar 2016 17:50:58 + 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 name RENAME ATTRIBUTE name RENAME TO new_name ALTER TYPE name SET SCHEMA new_schema ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ] +ALTER TYPE name RENAME VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ] where action is one of: @@ -123,6 +124,23 @@ ALTER TYPE name ADD VALUE [ IF NOT + +RENAME VALUE [ IF EXISTS ] TO [ IF NOT EXISTS ] + + + This form renames a value in an enum type. The value's place in the + enum's ordering is not affected. + + + If IF EXISTS or IF NOT EXISTS 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. + + + + CASCADE @@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT new_enum_value -The new value to be added to an enum type's list of values. +The new value to be added to an enum type's list of values, +or to rename an existing one to. Like all enum literals, it needs to be quoted. @@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT The existing enum value that the new value should be added immediately -before or after in the enum type's sort ordering. +before or after in the enum type's sort ordering, +or renamed from. Like all enum literals, it needs to be quoted. diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index 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; + boolfound_new = false; + + /* check length of new label is ok */ + if (strlen(newVal) > (NAMEDATALEN - 1)) + ereport(ERROR, +(errcode(ERRCODE_INVALID_NAME), + errmsg("invalid
Re: [HACKERS] [PATCH] Alter or rename enum value
Marko Tiikkaja writes: > On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote: >> ilm...@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 with the consequences of." -- Skud's Meta-Law -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Alter or rename enum value
On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote: ilm...@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. +ALTER VALUE [ IF EXISTST ] TO [ IF NOT EXISTS ] typo EXISTST + If IF EXISTS or is IF NOT EXISTS + 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Alter or rename enum value
ilm...@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?= Date: Thu, 24 Mar 2016 17:50:58 + 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 name RENAME ATTRIBUTE name RENAME TO new_name ALTER TYPE name SET SCHEMA new_schema ALTER TYPE name ADD VALUE [ IF NOT EXISTS ] new_enum_value [ { BEFORE | AFTER } existing_enum_value ] +ALTER TYPE name ALTER VALUE [ IF EXISTS ] existing_enum_value TO new_enum_value [ IF NOT EXISTS ] where action is one of: @@ -124,6 +125,23 @@ ALTER TYPE name ADD VALUE [ IF NOT +ALTER VALUE [ IF EXISTST ] TO [ IF NOT EXISTS ] + + + This form renames a value in an enum type. The value's place in the + enum's ordering is not affected. + + + If IF EXISTS or is IF NOT EXISTS + 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. + + + + + CASCADE @@ -241,7 +259,8 @@ ALTER TYPE name ADD VALUE [ IF NOT new_enum_value -The new value to be added to an enum type's list of values. +The new value to be added to an enum type's list of values, +or to rename an existing one to. Like all enum literals, it needs to be quoted. @@ -252,7 +271,8 @@ ALTER TYPE name ADD VALUE [ IF NOT The existing enum value that the new value should be added immediately -before or after in the enum type's sort ordering. +before or after in the enum type's sort ordering, +or renamed from. Like all enum literals, it needs to be quoted. diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index 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; + boolfound_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 v