Re: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value
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
> 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
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
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
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
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