From 984c92074d84a81dc17e9865fc79e264eb50ad61 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 26 Sep 2017 13:12:13 -0400 Subject: [PATCH] Remove heuristic same-transaction test from check_safe_enum_use(). The blacklist mechanism added by the preceding commit directly fixes most of the practical cases that the same-transaction test was meant to cover. What remains is use-cases like begin; create type e as enum('x'); alter type e add value 'y'; -- use 'y' somehow commit; However, because the same-transaction test is heuristic, it fails on small variants of that, such as renaming the type or changing its owner. Rather than try to explain the behavior to users, let's remove it and just have a rule that the newly added value can't be used before being committed, full stop. Perhaps later it will be worth the implementation effort and overhead to have a more accurate test for type-was-created-in-this-transaction. We'll wait for some field experience with v10 before deciding to do that. Back-patch to v10. Discussion: https://postgr.es/m/20170922185904.1448.16585@wrigleys.postgresql.org --- doc/src/sgml/ref/alter_type.sgml | 3 +-- src/backend/utils/adt/enum.c | 41 +++++++----------------------- src/test/regress/expected/enum.out | 18 ++++++------- src/test/regress/sql/enum.sql | 11 ++++---- 4 files changed, 24 insertions(+), 49 deletions(-) diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml index 446e08b175..7e2258e1e3 100644 --- a/doc/src/sgml/ref/alter_type.sgml +++ b/doc/src/sgml/ref/alter_type.sgml @@ -292,8 +292,7 @@ ALTER TYPE name RENAME VALUE If ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) is executed inside a transaction block, the new value cannot - be used until after the transaction has been committed, except in the case - that the enum type itself was created earlier in the same transaction. + be used until after the transaction has been committed. diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c index 401e7299fa..c0124f497e 100644 --- a/src/backend/utils/adt/enum.c +++ b/src/backend/utils/adt/enum.c @@ -48,7 +48,14 @@ static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper); * 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.) + * We don't implement that fully right now, but we do allow free use of enum + * values created during CREATE TYPE AS ENUM, which are surely of the same + * lifespan as the enum type. (This case is required by "pg_restore -1".) + * Values added by ALTER TYPE ADD VALUE are currently restricted, but could + * be allowed if the enum type could be proven to have been created earlier + * in the same transaction. (Note that comparing tuple xmins would not work + * for that, because the type tuple might have been updated in the current + * transaction. Subtransactions also create hazards to be accounted for.) * * This function needs to be called (directly or indirectly) in any of the * functions below that could return an enum value to SQL operations. @@ -58,7 +65,6 @@ 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 @@ -85,40 +91,11 @@ check_safe_enum_use(HeapTuple enumval_tup) if (!EnumBlacklisted(HeapTupleGetOid(enumval_tup))) 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 to be a subcommitted child of the enum value's xact, but not vice - * versa (since we might now be in a subxact of the type's originating - * xact, which could roll back along with the enum value's subxact). The - * former case seems a sufficiently weird usage pattern as to not be worth - * spending code for, so we're left with a simple equality check. - * - * We also insist that the type's pg_type row not be HEAP_UPDATED. If it - * is, we can't tell whether the row was created or only modified in the - * apparent originating xact, so it might be older than that xact. (We do - * not worry whether the enum value is HEAP_UPDATED; if it is, we might - * think it's too new and throw an unnecessary error, but we won't allow - * an unsafe case.) - */ - if (xmin == HeapTupleHeaderGetXmin(enumtyp_tup->t_data) && - !(enumtyp_tup->t_data->t_infomask & HEAP_UPDATED)) - { - /* same (sub)transaction, so safe */ - ReleaseSysCache(enumtyp_tup); - return; - } - /* * There might well be other tests we could do here to narrow down the * unsafe conditions, but for now just raise an exception. */ + en = (Form_pg_enum) GETSTRUCT(enumval_tup); ereport(ERROR, (errcode(ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE), errmsg("unsafe use of new value \"%s\" of enum type %s", diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out index 6bbe488736..4f839ce027 100644 --- a/src/test/regress/expected/enum.out +++ b/src/test/regress/expected/enum.out @@ -656,18 +656,16 @@ select enum_range(null::bogon); (1 row) ROLLBACK; --- check that we can add new values to existing enums in a transaction --- and use them, if the type is new as well +-- ideally, we'd allow this usage; but it requires keeping track of whether +-- the enum type was created in the current transaction, which is expensive BEGIN; CREATE TYPE bogus AS ENUM('good'); -ALTER TYPE bogus ADD VALUE 'bad'; -ALTER TYPE bogus ADD VALUE 'ugly'; -SELECT enum_range(null::bogus); - enum_range ------------------ - {good,bad,ugly} -(1 row) - +ALTER TYPE bogus RENAME TO bogon; +ALTER TYPE bogon ADD VALUE 'bad'; +ALTER TYPE bogon ADD VALUE 'ugly'; +select enum_range(null::bogon); -- fails +ERROR: unsafe use of new value "bad" of enum type bogon +HINT: New enum values must be committed before they can be used. ROLLBACK; -- -- Cleanup diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql index eb464a72c5..6affd0d1eb 100644 --- a/src/test/regress/sql/enum.sql +++ b/src/test/regress/sql/enum.sql @@ -315,13 +315,14 @@ ALTER TYPE bogus RENAME TO bogon; select enum_range(null::bogon); ROLLBACK; --- check that we can add new values to existing enums in a transaction --- and use them, if the type is new as well +-- ideally, we'd allow this usage; but it requires keeping track of whether +-- the enum type was created in the current transaction, which is expensive BEGIN; CREATE TYPE bogus AS ENUM('good'); -ALTER TYPE bogus ADD VALUE 'bad'; -ALTER TYPE bogus ADD VALUE 'ugly'; -SELECT enum_range(null::bogus); +ALTER TYPE bogus RENAME TO bogon; +ALTER TYPE bogon ADD VALUE 'bad'; +ALTER TYPE bogon ADD VALUE 'ugly'; +select enum_range(null::bogon); -- fails ROLLBACK; --