Re: Transactional enum additions - was Re: Alter or rename enum value

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.

Thanks!

Greetings,
Matthias

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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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

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 block

To 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_value
and
ALTER TYPE name RENAME VALUE [ IF EXISTS ] old_enum_value_name TO
new_enum_value_name

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.

I did some digging and maybe following commits are useful in this context:
7b90469b71761d240bf5efe3ad5bbd228429278e
c9e2e2db5c2090a880028fd8c1debff474640f50

Also there are these discussions where some of the messages contain some
useful information:
http://www.postgresql.org/message-id/29F36C7C98AB09499B1A209D48EAA615B7653DBC8A@mail2a.alliedtesting.com
http://www.postgresql.org/message-id/[email protected]
http://www.postgresql.org/message-id/[email protected]

Also have a look at this workaround:
http://en.dklab.ru/lib/dklab_postgresql_enum/

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,
Matthias

On 9 March 2016 at 18:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Andrew Dunstan <andrew(at)dunslane(dot)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
>

On 9 March 2016 at 20:19, Matthias Kurz <m(dot)kurz(at)irregular(dot)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 block
>
> To 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_value
> and
> ALTER TYPE name RENAME VALUE [ IF EXISTS ] old_enum_value_name TO
> new_enum_value_name
>
> 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.
>
> I did some digging and maybe following commits are useful in this context:
> 7b90469b71761d240bf5efe3ad5bbd228429278e
> c9e2e2db5c2090a880028fd8c1debff474640f50
>
> Also there are these discussions where some of the messages contain some
> useful information:
>
> http://www.postgresql.org/message-id/29F36C7C98AB09499B1A209D48EAA615B7653DBC8A@mail2a.alliedtesting.com
> http://www.postgresql.org/message-id/[email protected]
>
> http://www.postgresql.org/message-id/[email protected]
>
> Also have a look at this workaround:
> http://en.dklab.ru/lib/dklab_postgresql_enum/
>
> 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,
> Matthias
>
> On 9 March 2016 at 18:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)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(dot)kurz(at)irregular(dot)at> writes:

[altering and dropping enum values]

>>> Andrew Dunstan <andrew(at)dunslane(dot)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

ilmari(at)ilmari(dot)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(at)ilmari(dot)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(dot)Nasby(at)BlueTreble(dot)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 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.)

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

>
> 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.
>

Thanks for all your explanation!
We work a lot with enums and sometimes there are cases where we would like
to be able to add a new value or rename an existing value (in a new
transaction) - dropping isn't needed that much, but would be a bonus.
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 have to use
ugly hacks to make renaming/dropping work.
I didn't know implementing these features would 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 provide a patch myself unfortunately.
Hopefully at the commitfest at least the transaction limitation will/could
be tackled - that would help us a lot already.

Thanks for your time!

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(at)dunslane(dot)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 database migration tools. Even a very brutal solution like marking indexes containing the altered type invalid on a ROLLBACK would be preferable to the current situation.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

Andrew Dunstan <andrew(at)dunslane(dot)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.

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(at)dunslane(dot)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 database migration tools. Even a very brutal solution like marking indexes containing the altered type invalid on a ROLLBACK would 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(at)dunslane(dot)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...

David J.

On 03/26/2016 12:35 AM, David G. Johnston wrote:
> On Friday, March 25, 2016, Andrew Dunstan <andrew(at)dunslane(dot)net
> <mailto:andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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 enum 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.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 03/27/2016 12:43 AM, Christophe Pettus wrote:
> On Mar 26, 2016, at 7:40 AM, Andrew Dunstan <andrew(at)dunslane(dot)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 enum 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(at)dunslane(dot)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(at)sss(dot)pgh(dot)pa(dot)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 (inserting a new enum value, then populating a dimension table for it) would have to be done as two migrations rather than one, but that is much more doable in most tools than having a migration run without a transaction at all.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

ilmari(at)ilmari(dot)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)

On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote:
> ilmari(at)ilmari(dot)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(at)joh(dot)to> writes:

> On 2016-03-27 19:30, Dagfinn Ilmari Mannsåker wrote:
>> ilmari(at)ilmari(dot)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

> 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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)sss(dot)pgh(dot)pa(dot)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 migration as part of software releases. We’re using a db migration tool that understands that postgres can do most schema changes in a transaction, so it attempts to run our add enum value statements in a transaction, even if we mark them as individual changes. With some huffing and puffing we’ve managed to work around this limitation in the tool, but it would definitely be nice to keep our enum-related migrations with our other changes and drop the custom non-transactional migration 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 fine for us. In the (so far unprecedented) case that we need to use the new value in a migration at the same time, we’d always have 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(at)dunslane(dot)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(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:

> ilmari(at)ilmari(dot)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?

> 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/[email protected]

> 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(at)hasegeli(dot)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(at)hasegeli(dot)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.

> 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(at)hasegeli(dot)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(at)dunslane(dot)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

> 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(at)hasegeli(dot)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(at)hasegeli(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
>> On Mon, Sep 5, 2016 at 11:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)dunslane(dot)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(at)hasegeli(dot)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).

ilmari(at)ilmari(dot)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

On 7 September 2016 at 22:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> ilmari(at)ilmari(dot)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 <name> DROP VALUE <somevalue>" command?

Thank you!
Regards, Matthias

> 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(dot)Nasby(at)BlueTreble(dot)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