Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Tom Lane]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Emre Hasegeli]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  writes:
> OK, did that. Here is a patch that is undocumented but I think is 
> otherwise complete. It's been tested a bit and we haven't been able to 
> break it. Comments welcome.

Got around to looking at this.  Attached is a revised version that I think
is in committable shape.  The main non-cosmetic change is that the test
for "type was created in same transaction as new value" now consists of
comparing the xmins of the pg_type and pg_enum rows, without consulting
GetCurrentTransactionId().  I did not like the original coding because
it would pointlessly disallow references to enum values that were created
in a parent transaction of the current subxact.  This way is still leaving
some subxact use-cases on the table, as noted in the code comments, but
it's more flexible than before.

Barring objections I'll push this soon.

regards, tom lane

diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..aec73f6 100644
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
*** ALTER TYPE t_data) == GetCurrentTransactionId() &&
- 		!(tup->t_data->t_infomask & HEAP_UPDATED))
- 		 /* safe to do inside transaction block */ ;
- 	else
- 		PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD");
- 
  	/* Check it's an enum and check user has permission to ALTER the enum */
  	checkEnumOwner(tup);
  
--- 1236,1241 
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ac50c2a..ac64135 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*** ProcessUtilitySlow(Node *parsetree,
*** 1359,1365 
  break;
  
  			case T_AlterEnumStmt:		/* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
  break;
  
  			case T_ViewStmt:	/* CREATE VIEW */
--- 1359,1365 
  break;
  
  			case T_AlterEnumStmt:		/* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree);
  break;
  
  			case T_ViewStmt:	/* CREATE VIEW */
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 135a544..47d5355 100644
*** a/src/backend/utils/adt/enum.c
--- b/src/backend/utils/adt/enum.c
***
*** 19,24 
--- 19,25 
  #include "catalog/indexing.h"
  #include "catalog/pg_enum.h"
  #include "libpq/pqformat.h"
+ #include "storage/procarray.h"
  #include "utils/array.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
*** static Oid	enum_endpoint(Oid enumtypoid,
*** 31,36 
--- 32,124 
  static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
  
  
+ /*
+  * Disallow use of an uncommitted pg_enum tuple.
+  *
+  * We need to make sure that uncommitted enum values don't get into indexes.
+  * If they did, and if we then rolled back the pg_enum addition, we'd have
+  * broken the index because value comparisons will not work reliably without
+  * an underlying pg_enum entry.  (Note that removal of the heap entry
+  * containing an enum value is not sufficient to ensure that it doesn't appear
+  * in upper levels of indexes.)  To do this we prevent an uncommitted row from
+  * being used for any SQL-level purpose.  This is stronger than necessary,
+  * since the value might not be getting inserted into a table or there might
+  * be no index on its column, but it's easy to enforce centrally.
+  *
+  * However, it's okay to allow use of uncommitted values belonging to enum
+  * types that were themselves created in the same transaction, because then
+  * any such index would also be new and would go away altogether on rollback.
+  * (This case is required by pg_upgrade.)
+  *
+  * This function needs to be called (directly or indirectly) in any of the
+  * functions below that could return an enum value to SQL operations.
+  */
+ static void
+ check_safe_enum_use(HeapTuple enumval_tup)
+ {
+ 	TransactionId xmin;
+ 	Form_pg_enum en;
+ 	HeapTuple	enumtyp_tup;
+ 
+ 	/*
+ 	 * If the row is hinted as committed, it's surely safe.  This provides a
+ 	 * fast path for all normal use-cases.
+ 	 */
+ 	if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
+ 		return;
+ 
+ 	/*
+ 	 * Usually, a row would get hinted as committed when it's read or loaded
+ 	 * into syscache; but just in case not, let's check the xmin directly.
+ 	 */
+ 	xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
+ 	if (!TransactionIdIsInProgress(xmin) &&
+ 		TransactionIdDidCommit(xmin))
+ 		return;
+ 
+ 	/* It is a new enum value, so check to see if the whole enum is new */
+ 	en = (Form_pg_enum) GETSTRUCT(enumval_tup);
+ 	enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));
+ 	if (!HeapTupleIsValid(enumtyp_tup))
+ 		elog(ERROR, "cache lookup failed for type %u", en->enumtypid);
+ 
+ 	/*
+ 	 * We insist that the type have been created in the same (sub)transaction
+ 	 * as the enum value.  It would be safe to allow the type's originating
+ 	 * xact

Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Andrew Dunstan]

On 04/02/2016 01:20 PM, Tom Lane wrote:

Andrew Dunstan  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


transactional_enum-additions-v1x.patch
Description: binary/octet-stream

-- 
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] Alter or rename enum value

[Tom Dunstan]

Just stumbled across this thread while looking for something else…

> On 28 Mar 2016, at 12:50 AM, Tom Lane  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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Andrew Dunstan]

On 03/29/2016 04:56 PM, Andrew Dunstan wrote:



On 03/27/2016 10:20 AM, Tom Lane wrote:

Andrew Dunstan  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





--
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] Alter or rename enum value

[Andrew Dunstan]

On 03/27/2016 10:20 AM, Tom Lane wrote:

Andrew Dunstan  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



--
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] Alter or rename enum value

[Jim Nasby]

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


--
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] Alter or rename enum value

[Emre Hasegeli]

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


-- 
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] Alter or rename enum value

[Christophe Pettus]

On Mar 27, 2016, at 7:20 AM, Tom Lane  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
   x...@thebuild.com



-- 
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] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  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


-- 
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] Alter or rename enum value

[Andrew Dunstan]

On 03/27/2016 12:43 AM, Christophe Pettus wrote:

On Mar 26, 2016, at 7:40 AM, Andrew Dunstan  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


--
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] Alter or rename enum value

[Christophe Pettus]

On Mar 26, 2016, at 7:40 AM, Andrew Dunstan  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
   x...@thebuild.com



-- 
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] Alter or rename enum value

[Andrew Dunstan]

On 03/26/2016 10:25 AM, Tom Lane wrote:

Andrew Dunstan  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



--
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] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  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


-- 
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] Alter or rename enum value

[Andrew Dunstan]

On 03/26/2016 12:35 AM, David G. Johnston wrote:
On Friday, March 25, 2016, Andrew Dunstan > 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



--
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] Alter or rename enum value

[David G. Johnston]

On Friday, March 25, 2016, Andrew Dunstan  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.

Re: [HACKERS] Alter or rename enum value

[Andrew Dunstan]

On 03/25/2016 03:22 PM, Christophe Pettus wrote:

On Mar 25, 2016, at 11:50 AM, Andrew Dunstan  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


--
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] Alter or rename enum value

[Jim Nasby]

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


--
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] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> On 03/25/2016 04:13 AM, Matthias Kurz wrote:
>>
>> Hopefully at the commitfest at least the transaction limitation
>> will/could be tackled - that would help us a lot already.
>
> I don't believe anyone knows how to do that safely. Enums pose special
> problems here exactly because unlike all other types the set of valid
> values is mutable.

However, this problem (and the one described in the comments of
AlterEnum()) doesn't apply to altering the name, since that doesn't
affect the OID or the ordering.  Attached is version 2 of the patch,
which allows ALTER TYPE ... ALTER VALUE inside a transaction.

It still needs documentation, and possibly support for IF (NOT) EXISTS,
if people think that's useful.

>From 0a482f6d7434964ce51f66c260bde4a74dac4da0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Unlike adding values, altering a value can be done in a transaction,
since it doesn't affect the OID values or ordering, so is safe to
rollback.

TODO: documentation, IF (NOT) EXISTS support(?)
---
 src/backend/catalog/pg_enum.c  | 88 ++
 src/backend/commands/typecmds.c| 50 --
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 58 +
 src/test/regress/sql/enum.sql  | 27 
 7 files changed, 218 insertions(+), 22 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..1079e6c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,94 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	old_tup = NULL;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+	boolfound_new = false;
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename and check if the new label is already in
+	 * use.  The unique index on pg_enum would catch this anyway, but we
+	 * prefer a friendlier error message.
+	 */
+	for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+	{
+		enum_tup = &(list->members[nbr_index]->tuple);
+		nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+		if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+			old_tup = enum_tup;
+
+		if (strcmp(NameStr(nbr_en->enumlabel), newVal) == 0)
+			found_new = true;
+	}
+	if (!old_tup)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+		oldVal)));
+	if (found_new)
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("enum label \"%s\" already exists",
+		newVal)));
+
+	enum_tup = heap_copytuple(old_tup);
+	nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+	ReleaseCatCacheList(list);
+
+	/* Update new pg_enum entry */
+	namestrcpy(&nbr_en->enumlabel, newVal);
+	simple_heap_update(pg_enum, &enum_tup->t_self, enum_tup);
+	CatalogUpdateIndexes(pg_enum, enum_tup);
+	heap_freetuple(enum_tup);
+
+	/* Make the updates visible */
+	CommandCounterIncrement();
+
+	heap_close(pg_enum, RowExclusiveLock);
+}
+
+
+/*
  * RenumberEnumType
  *		Renumber existing enum elements to have sort positions 1..n.
  *
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 227d382..63f030b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1236,32 +1236,38 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
 	if (!HeapTupleIsValid(tup))
 		elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
 
-	/*
-	 * Ordinarily we disallow ad

Re: [HACKERS] Alter or rename enum value

[Christophe Pettus]

On Mar 25, 2016, at 11:50 AM, Andrew Dunstan  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
   x...@thebuild.com



-- 
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] Alter or rename enum value

[Gavin Flower]

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


--
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] Alter or rename enum value

[Jim Nasby]

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


--
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] Alter or rename enum value

[Andrew Dunstan]

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



--
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] Alter or rename enum value

[Matthias Kurz]

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

Re: [HACKERS] Alter or rename enum value

[Tom Lane]

Jim Nasby  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


-- 
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] Alter or rename enum value

[Jim Nasby]

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


--
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] Alter or rename enum value

[Matthias Kurz]

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

Re: [HACKERS] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

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

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



-- 
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] Alter or rename enum value

[Dagfinn Ilmari Mannsåker]

Matthias Kurz  writes:

[altering and dropping enum values]

>>> Andrew Dunstan  writes:
>>> > On 03/09/2016 11:07 AM, Tom Lane wrote:
>>> >> I have a vague recollection that we discussed this at the time the enum
>>> >> stuff went in, and there are concurrency issues?  Don't recall details
>>> >> though.
>>>
>>> > Rings a vague bell, but should it be any worse than adding new labels?
>>>
>>> I think what I was recalling is the hazards discussed in the comments for
>>> RenumberEnumType.  However, the problem there is that a backend could make
>>> inconsistent ordering decisions due to seeing two different pg_enum rows
>>> under different snapshots.  Updating a single row to change its name
>>> doesn't seem to have a comparable hazard, and it wouldn't affect ordering
>>> anyway.  So it's probably no worse than any other object-rename situation.
>>>
>>> regards, tom lane
>>>
>>
>>
> Is there a way or a procedure we can go through to make the these ALTER
> TYPE enhancements a higher priority? How do you choose which
> features/enhancements to implement (next)?

I was bored and thought "how hard could it be?", and a few hours'
hacking later, I have something that seems to work.  It doesn't do IF
NOT EXISTS yet, and the error messaging could do with some improvement,
and there are no docs.  The patch is attached, as well as at
https://github.com/ilmari/postgres/commit/enum-alter-value

>From 1cb8feac0179eaa44720822ad84e146ec3ba67ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Thu, 24 Mar 2016 17:50:58 +
Subject: [PATCH] Add ALTER TYPE ... ALTER VALUE '...' TO '...'

Needs docs, and when altering a non-existent value to one that already
exists, it should probably complain about the former fact, not the
latter.
---
 src/backend/catalog/pg_enum.c  | 93 ++
 src/backend/commands/typecmds.c| 17 +--
 src/backend/parser/gram.y  | 14 ++
 src/include/catalog/pg_enum.h  |  2 +
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/enum.out | 22 +
 src/test/regress/sql/enum.sql  | 11 +
 7 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index af89daa..81e170c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -464,6 +464,99 @@ restart:
 
 
 /*
+ * RenameEnumLabel
+ *		Add a new label to the enum set. By default it goes at
+ *		the end, but the user can choose to place it before or
+ *		after any existing set member.
+ */
+void
+RenameEnumLabel(Oid enumTypeOid,
+const char *oldVal,
+const char *newVal)
+{
+	Relation	pg_enum;
+	HeapTuple	enum_tup;
+	CatCList   *list;
+	int			nelems;
+	int			nbr_index;
+	Form_pg_enum nbr_en;
+
+
+
+	/* check length of new label is ok */
+	if (strlen(newVal) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid enum label \"%s\"", newVal),
+ errdetail("Labels must be %d characters or less.",
+		   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Without that, we couldn't be sure to get a consistent view
+	 * of the enum members via the syscache.  Note that this does not block
+	 * other backends from inspecting the type; see comments for
+	 * RenumberEnumType.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	/*
+	 * Check if label is already in use.  The unique index on pg_enum would
+	 * catch this anyway, but we prefer a friendlier error message.
+	 */
+	enum_tup = SearchSysCache2(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid),
+			   CStringGetDatum(newVal));
+	if (HeapTupleIsValid(enum_tup))
+	{
+		ReleaseSysCache(enum_tup);
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("enum label \"%s\" already exists",
+		newVal)));
+	}
+
+	pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+			   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/* Locate the element to rename */
+	for (nbr_index = 0; nbr_index < nelems; nbr_index++)
+	{
+		enum_tup = &(list->members[nbr_index]->tuple);
+		nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+		if (strcmp(NameStr(nbr_en->enumlabel), oldVal) == 0)
+			break;
+	}
+	if (nbr_index >= nelems)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+		oldVal)));
+
+	enum_tup = heap_copytuple(enum_tup);
+	nbr_en = (Form_pg_enum) GETSTRUCT(enum_tup);
+	ReleaseCatCacheList(list);
+
+	/* Update new pg_enum entry */
+	namestrcpy(&nbr_en->enumlabel, newVal);
+	simple_heap_update(pg_enum, &enum_tup->t_self, enum_tup);
+	CatalogUpdateIndexes(pg_enum

Re: [HACKERS] Alter or rename enum value

[Matthias Kurz]

On 9 March 2016 at 20:19, Matthias Kurz  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/29f36c7c98ab09499b1a209d48eaa615b7653db...@mail2a.alliedtesting.com
> http://www.postgresql.org/message-id/50324f26.3090...@dunslane.net
>
> http://www.postgresql.org/message-id/20130819122938.gb8...@alap2.anarazel.de
>
> 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  wrote:
>
>> Andrew Dunstan  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)?

Re: [HACKERS] Alter or rename enum value

[Matthias Kurz]

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/29f36c7c98ab09499b1a209d48eaa615b7653db...@mail2a.alliedtesting.com
http://www.postgresql.org/message-id/50324f26.3090...@dunslane.net
http://www.postgresql.org/message-id/20130819122938.gb8...@alap2.anarazel.de

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  wrote:

> Andrew Dunstan  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
>

Re: [HACKERS] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  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


-- 
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] Alter or rename enum value

[Andrew Dunstan]

On 03/09/2016 11:07 AM, Tom Lane wrote:

Andrew Dunstan  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


--
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] Alter or rename enum value

[Tom Lane]

Andrew Dunstan  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


-- 
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] Alter or rename enum value

[Andrew Dunstan]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers