diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 71d4df9..13231bb 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -1237,23 +1237,13 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) elog(ERROR, "cache lookup failed for type %u", enum_type_oid); /* - * 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.) We test this by - * seeing if the pg_type row has xmin == current XID and is not - * HEAP_UPDATED. If it is HEAP_UPDATED, we can't be sure whether the type - * was created or only modified in this xact. So we are disallowing some - * cases that could theoretically be safe; but fortunately pg_dump only - * needs the simplest case. - */ - if (HeapTupleHeaderGetXmin(tup->t_data) == GetCurrentTransactionId() && - !(tup->t_data->t_infomask & HEAP_UPDATED)) - /* safe to do inside transaction block */ ; - else - PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD"); + * We used to prevent this from happening inside a transaction block in + * situations where the new value could get into an upper level index page + * and cause corruption on rollback. Now we instead detect use of the new + * value in such situations and raise an exception, so that just adding to + * an enum in a transaction block is now safe. + * See enum.c:check_safe_enum_use() + */ /* Check it's an enum and check user has permission to ALTER the enum */ checkEnumOwner(tup); diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c index 135a544..7e0977e 100644 --- a/src/backend/utils/adt/enum.c +++ b/src/backend/utils/adt/enum.c @@ -16,6 +16,7 @@ #include "access/genam.h" #include "access/heapam.h" #include "access/htup_details.h" +#include "access/xact.h" #include "catalog/indexing.h" #include "catalog/pg_enum.h" #include "libpq/pqformat.h" @@ -30,6 +31,60 @@ static Oid enum_endpoint(Oid enumtypoid, ScanDirection direction); static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper); +/* + * We need to make sure that new enum values don't get into the upper levels + * of indexes where we can't roll them back. To do that we prevent the use + * of enum values where the enum row is not committed, unless the type itself + * has been created in the current transaction. In the latter case any + * rollback would involve rolling back the whole index, since it couldn't + * exist before the enum type, and thus we couldn't get a corrupted index. + * We test this by seeing if the pg_type row has xmin == current XID and is not + * HEAP_UPDATED. If it is HEAP_UPDATED, we can't be sure whether the type + * was created or only modified in this xact. So we are disallowing some + * cases that could theoretically be safe. Narrowing down the unsafe conditions + * is left as an exercise for another day. + * + * This function needs to be called directly or indirectly in any of the + * functions below that could produce an enum value, particulary, enum_in, + * enum_recv, enum_first, enum_last, and all the enum_range variants. + */ +static void +check_safe_enum_use(char * name, Oid enumtypoid, HeapTuple enum_tup) +{ + /* First see if it's a new enum value - if not it's safe */ + if (!HeapTupleHeaderXminCommitted(enum_tup->t_data)) + { + + /* + * It is a new enum value, so now check to see if the whole enum is + * new. If it is, we're safe. + */ + HeapTuple tup; + + tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(enumtypoid)); + Assert(HeapTupleIsValid(tup)); + + if (HeapTupleHeaderGetXmin(tup->t_data) ==GetCurrentTransactionId() && + !(tup->t_data->t_infomask & HEAP_UPDATED)) + /* safe, must have been created in this transaction */ ; + else + /* + * We can't be in a top level transaction here, because the + * enum row hasn't been committed. There might well be other + * tests we could do here to narrow down the unsafe conditions, + * but for now just raise an exception. + */ + ereport(ERROR, + (errcode(ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE), + errmsg("unsafe use of new value for enum %s: \"%s\"", + format_type_be(enumtypoid), + name), + errhint("New enum values must be committed before they can be used."))); + + ReleaseSysCache(tup); + } +} + /* Basic I/O support */ @@ -59,6 +114,8 @@ enum_in(PG_FUNCTION_ARGS) format_type_be(enumtypoid), name))); + check_safe_enum_use(name, enumtypoid, tup); + /* * This comes from pg_enum.oid and stores system oids in user tables. This * oid must be preserved by binary upgrades. @@ -124,6 +181,8 @@ enum_recv(PG_FUNCTION_ARGS) format_type_be(enumtypoid), name))); + check_safe_enum_use(name, enumtypoid, tup); + enumoid = HeapTupleGetOid(tup); ReleaseSysCache(tup); @@ -327,9 +386,18 @@ enum_endpoint(Oid enumtypoid, ScanDirection direction) enum_tuple = systable_getnext_ordered(enum_scan, direction); if (HeapTupleIsValid(enum_tuple)) + { + Form_pg_enum en; + + en = (Form_pg_enum) GETSTRUCT(enum_tuple); + check_safe_enum_use(NameStr(en->enumlabel), enumtypoid, enum_tuple); minmax = HeapTupleGetOid(enum_tuple); + } else + { + /* should only happen with an empty enum */ minmax = InvalidOid; + } systable_endscan_ordered(enum_scan); index_close(enum_idx, AccessShareLock); @@ -490,6 +558,11 @@ enum_range_internal(Oid enumtypoid, Oid lower, Oid upper) if (left_found) { + Form_pg_enum en; + + en = (Form_pg_enum) GETSTRUCT(enum_tuple); + check_safe_enum_use(NameStr(en->enumlabel), enumtypoid, enum_tuple); + if (cnt >= max) { max *= 2; diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt index be924d5..c6495d2 100644 --- a/src/backend/utils/errcodes.txt +++ b/src/backend/utils/errcodes.txt @@ -199,6 +199,7 @@ Section: Class 22 - Data Exception 22P03 E ERRCODE_INVALID_BINARY_REPRESENTATION invalid_binary_representation 22P04 E ERRCODE_BAD_COPY_FILE_FORMAT bad_copy_file_format 22P05 E ERRCODE_UNTRANSLATABLE_CHARACTER untranslatable_character +22P07 E ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE unsafe_new_enum_value_usage 2200L E ERRCODE_NOT_AN_XML_DOCUMENT not_an_xml_document 2200M E ERRCODE_INVALID_XML_DOCUMENT invalid_xml_document 2200N E ERRCODE_INVALID_XML_CONTENT invalid_xml_content diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out index 1a61a5b..a89fc30 100644 --- a/src/test/regress/expected/enum.out +++ b/src/test/regress/expected/enum.out @@ -560,25 +560,55 @@ DROP TYPE bogus; -- check transactional behaviour of ALTER TYPE ... ADD VALUE -- CREATE TYPE bogus AS ENUM('good'); --- check that we can't add new values to existing enums in a transaction +-- check that we can add new values to existing enums in a transaction +-- but we can't use them BEGIN; ALTER TYPE bogus ADD VALUE 'bad'; -ERROR: ALTER TYPE ... ADD cannot run inside a transaction block +SAVEPOINT x; +SELECT 'bad'::bogus; +ERROR: unsafe use of new value for enum bogus: "bad" +LINE 1: SELECT 'bad'::bogus; + ^ +HINT: New enum values must be committed before they can be used. +ROLLBACK TO x; +SELECT enum_first(null::bogus) as should_suceed; + should_suceed +--------------- + good +(1 row) + +SELECT enum_last(null::bogus); +ERROR: unsafe use of new value for enum bogus: "bad" +HINT: New enum values must be committed before they can be used. +ROLLBACK TO x; +SELECT enum_range(null::bogus); +ERROR: unsafe use of new value for enum bogus: "bad" +HINT: New enum values must be committed before they can be used. COMMIT; -- check that we recognize the case where the enum already existed but was -- modified in the current txn BEGIN; ALTER TYPE bogus RENAME TO bogon; -ALTER TYPE bogon ADD VALUE 'bad'; -ERROR: ALTER TYPE ... ADD cannot run inside a transaction block +ALTER TYPE bogon ADD VALUE 'alsobad'; +SELECT 'alsobad'::bogon; +ERROR: unsafe use of new value for enum bogon: "alsobad" +LINE 1: SELECT 'alsobad'::bogon; + ^ +HINT: New enum values must be committed before they can be used. ROLLBACK; DROP TYPE bogus; -- check that we *can* add new values to existing enums in a transaction, --- if the type is new as well +-- and to use them, if the type is new as well BEGIN; CREATE TYPE bogus AS ENUM(); ALTER TYPE bogus ADD VALUE 'good'; ALTER TYPE bogus ADD VALUE 'ugly'; +SELECT enum_range(null::bogus); + enum_range +------------- + {good,ugly} +(1 row) + ROLLBACK; -- -- Cleanup diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql index 88a835e..6203c44 100644 --- a/src/test/regress/sql/enum.sql +++ b/src/test/regress/sql/enum.sql @@ -262,26 +262,36 @@ DROP TYPE bogus; -- CREATE TYPE bogus AS ENUM('good'); --- check that we can't add new values to existing enums in a transaction +-- check that we can add new values to existing enums in a transaction +-- but we can't use them BEGIN; ALTER TYPE bogus ADD VALUE 'bad'; +SAVEPOINT x; +SELECT 'bad'::bogus; +ROLLBACK TO x; +SELECT enum_first(null::bogus) as should_suceed; +SELECT enum_last(null::bogus); +ROLLBACK TO x; +SELECT enum_range(null::bogus); COMMIT; -- check that we recognize the case where the enum already existed but was -- modified in the current txn BEGIN; ALTER TYPE bogus RENAME TO bogon; -ALTER TYPE bogon ADD VALUE 'bad'; +ALTER TYPE bogon ADD VALUE 'alsobad'; +SELECT 'alsobad'::bogon; ROLLBACK; DROP TYPE bogus; -- check that we *can* add new values to existing enums in a transaction, --- if the type is new as well +-- and to use them, if the type is new as well BEGIN; CREATE TYPE bogus AS ENUM(); ALTER TYPE bogus ADD VALUE 'good'; ALTER TYPE bogus ADD VALUE 'ugly'; +SELECT enum_range(null::bogus); ROLLBACK; --