Re: [HACKERS] [PATCH] Alter or rename enum value

[Tom Lane]

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

[Emre Hasegeli]

> 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

[Jim Nasby]

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

[Emre Hasegeli]

> 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

[Matthias Kurz]

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

[Tom Lane]

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

[Dagfinn Ilmari Mannsåker]

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 

Re: [HACKERS] [PATCH] Alter or rename enum value

[Emre Hasegeli]

> 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

[Tom Lane]

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

[Andrew Dunstan]

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

[Tom Lane]

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

[Robert Haas]

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

[Tom Lane]

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

[Emre Hasegeli]

> 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

[Tom Lane]

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

[Emre Hasegeli]

> 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

[Dagfinn Ilmari Mannsåker]

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

Re: [HACKERS] [PATCH] Alter or rename enum value

[Tom Lane]

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

[Emre Hasegeli]

> 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 Hasegeli]

> 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

[Dagfinn Ilmari Mannsåker]

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),
+	

Re: [HACKERS] [PATCH] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

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

[Marko Tiikkaja]

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