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 

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