From bbdae7181682dd6a52ff8ea05a99af6103fcdf6d Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Wed, 3 Oct 2018 13:44:14 +1300 Subject: [PATCH 1/2] Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux). Originally committed as 15bc038f9bcd1a9af3f625caffafc7c20322202d (plus some follow-ups), but then reverted in 93a1af0b3f63838774a9e524589344c3d44c867d. Andrew Dunstan and Tom Lane --- doc/src/sgml/ref/alter_type.sgml | 5 +- src/backend/access/transam/xact.c | 4 ++ src/backend/catalog/pg_enum.c | 64 +++++++++++++++++++++ src/backend/commands/typecmds.c | 29 ++-------- src/backend/tcop/utility.c | 2 +- src/backend/utils/adt/enum.c | 90 ++++++++++++++++++++++++++++++ src/backend/utils/errcodes.txt | 1 + src/include/catalog/pg_enum.h | 2 + src/include/commands/typecmds.h | 2 +- src/test/regress/expected/enum.out | 78 ++++++++++++++++++++++---- src/test/regress/sql/enum.sql | 42 +++++++++++--- 11 files changed, 270 insertions(+), 49 deletions(-) diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml index 9127dfd88de..67be1dd5683 100644 --- a/doc/src/sgml/ref/alter_type.sgml +++ b/doc/src/sgml/ref/alter_type.sgml @@ -290,8 +290,9 @@ ALTER TYPE name RENAME VALUE Notes - ALTER TYPE ... ADD VALUE (the form that adds a new value to an - enum type) cannot be executed inside a transaction block. + 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. diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 875be180fe4..a67539d099d 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -31,6 +31,7 @@ #include "access/xloginsert.h" #include "access/xlogutils.h" #include "catalog/namespace.h" +#include "catalog/pg_enum.h" #include "catalog/storage.h" #include "commands/async.h" #include "commands/tablecmds.h" @@ -2101,6 +2102,7 @@ CommitTransaction(void) AtCommit_Notify(); AtEOXact_GUC(true, 1); AtEOXact_SPI(true); + AtEOXact_Enum(); AtEOXact_on_commit_actions(true); AtEOXact_Namespace(true, is_parallel_worker); AtEOXact_SMgr(); @@ -2379,6 +2381,7 @@ PrepareTransaction(void) /* PREPARE acts the same as COMMIT as far as GUC is concerned */ AtEOXact_GUC(true, 1); AtEOXact_SPI(true); + AtEOXact_Enum(); AtEOXact_on_commit_actions(true); AtEOXact_Namespace(true, false); AtEOXact_SMgr(); @@ -2581,6 +2584,7 @@ AbortTransaction(void) AtEOXact_GUC(false, 1); AtEOXact_SPI(false); + AtEOXact_Enum(); AtEOXact_on_commit_actions(false); AtEOXact_Namespace(false, is_parallel_worker); AtEOXact_SMgr(); diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index fde361f3675..297d64a8106 100644 --- a/src/backend/catalog/pg_enum.c +++ b/src/backend/catalog/pg_enum.c @@ -28,6 +28,8 @@ #include "utils/builtins.h" #include "utils/catcache.h" #include "utils/fmgroids.h" +#include "utils/hsearch.h" +#include "utils/memutils.h" #include "utils/syscache.h" #include "utils/tqual.h" @@ -35,6 +37,17 @@ /* Potentially set by pg_upgrade_support functions */ Oid binary_upgrade_next_pg_enum_oid = InvalidOid; +/* + * Hash table of enum value OIDs created during the current transaction by + * AddEnumLabel. We disallow using these values until the transaction is + * committed; otherwise, they might get into indexes where we can't clean + * them up, and then if the transaction rolls back we have a broken index. + * (See comments for check_safe_enum_use() in enum.c.) Values created by + * EnumValuesCreate are *not* blacklisted; we assume those are created during + * CREATE TYPE, so they can't go away unless the enum type itself does. + */ +static HTAB *enum_blacklist = NULL; + static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems); static int sort_order_cmp(const void *p1, const void *p2); @@ -460,6 +473,24 @@ restart: heap_freetuple(enum_tup); heap_close(pg_enum, RowExclusiveLock); + + /* Set up the blacklist hash if not already done in this transaction */ + if (enum_blacklist == NULL) + { + HASHCTL hash_ctl; + + memset(&hash_ctl, 0, sizeof(hash_ctl)); + hash_ctl.keysize = sizeof(Oid); + hash_ctl.entrysize = sizeof(Oid); + hash_ctl.hcxt = TopTransactionContext; + enum_blacklist = hash_create("Enum value blacklist", + 32, + &hash_ctl, + HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); + } + + /* Add the new value to the blacklist */ + (void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL); } @@ -547,6 +578,39 @@ RenameEnumLabel(Oid enumTypeOid, } +/* + * Test if the given enum value is on the blacklist + */ +bool +EnumBlacklisted(Oid enum_id) +{ + bool found; + + /* If we've made no blacklist table, all values are safe */ + if (enum_blacklist == NULL) + return false; + + /* Else, is it in the table? */ + (void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found); + return found; +} + + +/* + * Clean up enum stuff after end of top-level transaction. + */ +void +AtEOXact_Enum(void) +{ + /* + * Reset the blacklist table, as all our enum values are now committed. + * The memory will go away automatically when TopTransactionContext is + * freed; it's sufficient to clear our pointer. + */ + enum_blacklist = NULL; +} + + /* * 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 b018585aef8..3271962a7a7 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -1270,10 +1270,10 @@ DefineEnum(CreateEnumStmt *stmt) /* * AlterEnum - * ALTER TYPE on an enum. + * Adds a new label to an existing enum. */ ObjectAddress -AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) +AlterEnum(AlterEnumStmt *stmt) { Oid enum_type_oid; TypeName *typename; @@ -1291,6 +1291,8 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) /* Check it's an enum and check user has permission to ALTER the enum */ checkEnumOwner(tup); + ReleaseSysCache(tup); + if (stmt->oldVal) { /* Rename an existing label */ @@ -1299,27 +1301,6 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) else { /* Add a new label */ - - /* - * 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 - PreventInTransactionBlock(isTopLevel, "ALTER TYPE ... ADD"); - AddEnumLabel(enum_type_oid, stmt->newVal, stmt->newValNeighbor, stmt->newValIsAfter, stmt->skipIfNewValExists); @@ -1329,8 +1310,6 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel) ObjectAddressSet(address, TypeRelationId, enum_type_oid); - ReleaseSysCache(tup); - return address; } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index b5804f64ad4..898091c45f8 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1439,7 +1439,7 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_AlterEnumStmt: /* ALTER TYPE (enum) */ - address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel); + 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 562cd5c555d..01edccced29 100644 --- a/src/backend/utils/adt/enum.c +++ b/src/backend/utils/adt/enum.c @@ -19,6 +19,7 @@ #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" @@ -31,6 +32,79 @@ static Oid enum_endpoint(Oid enumtypoid, ScanDirection direction); 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. + * 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. + */ +static void +check_safe_enum_use(HeapTuple enumval_tup) +{ + TransactionId xmin; + Form_pg_enum en; + + /* + * 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; + + /* + * Check if the enum value is blacklisted. If not, it's safe, because it + * was made during CREATE TYPE AS ENUM and can't be shorter-lived than its + * owning type. (This'd also be false for values made by other + * transactions; but the previous tests should have handled all of those.) + */ + if (!EnumBlacklisted(HeapTupleGetOid(enumval_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", + NameStr(en->enumlabel), + format_type_be(en->enumtypid)), + errhint("New enum values must be committed before they can be used."))); +} + + /* Basic I/O support */ Datum @@ -59,6 +133,9 @@ enum_in(PG_FUNCTION_ARGS) format_type_be(enumtypoid), name))); + /* check it's safe to use in SQL */ + check_safe_enum_use(tup); + /* * This comes from pg_enum.oid and stores system oids in user tables. This * oid must be preserved by binary upgrades. @@ -124,6 +201,9 @@ enum_recv(PG_FUNCTION_ARGS) format_type_be(enumtypoid), name))); + /* check it's safe to use in SQL */ + check_safe_enum_use(tup); + enumoid = HeapTupleGetOid(tup); ReleaseSysCache(tup); @@ -331,9 +411,16 @@ enum_endpoint(Oid enumtypoid, ScanDirection direction) enum_tuple = systable_getnext_ordered(enum_scan, direction); if (HeapTupleIsValid(enum_tuple)) + { + /* check it's safe to use in SQL */ + check_safe_enum_use(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); @@ -494,6 +581,9 @@ enum_range_internal(Oid enumtypoid, Oid lower, Oid upper) if (left_found) { + /* check it's safe to use in SQL */ + check_safe_enum_use(enum_tuple); + if (cnt >= max) { max *= 2; diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt index 29efb3f6efc..788f88129bd 100644 --- a/src/backend/utils/errcodes.txt +++ b/src/backend/utils/errcodes.txt @@ -401,6 +401,7 @@ Section: Class 55 - Object Not In Prerequisite State 55006 E ERRCODE_OBJECT_IN_USE object_in_use 55P02 E ERRCODE_CANT_CHANGE_RUNTIME_PARAM cant_change_runtime_param 55P03 E ERRCODE_LOCK_NOT_AVAILABLE lock_not_available +55P04 E ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE unsafe_new_enum_value_usage Section: Class 57 - Operator Intervention diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h index e6fd41e5b23..a70a05cc64c 100644 --- a/src/include/catalog/pg_enum.h +++ b/src/include/catalog/pg_enum.h @@ -52,5 +52,7 @@ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal, bool skipIfExists); extern void RenameEnumLabel(Oid enumTypeOid, const char *oldVal, const char *newVal); +extern bool EnumBlacklisted(Oid enum_id); +extern void AtEOXact_Enum(void); #endif /* PG_ENUM_H */ diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h index a04f3405de1..ac4ce50ef61 100644 --- a/src/include/commands/typecmds.h +++ b/src/include/commands/typecmds.h @@ -26,7 +26,7 @@ extern void RemoveTypeById(Oid typeOid); extern ObjectAddress DefineDomain(CreateDomainStmt *stmt); extern ObjectAddress DefineEnum(CreateEnumStmt *stmt); extern ObjectAddress DefineRange(CreateRangeStmt *stmt); -extern ObjectAddress AlterEnum(AlterEnumStmt *stmt, bool isTopLevel); +extern ObjectAddress AlterEnum(AlterEnumStmt *stmt); extern ObjectAddress DefineCompositeType(RangeVar *typevar, List *coldeflist); extern Oid AssignTypeArrayOid(void); diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out index a0b81608a1b..4f839ce0279 100644 --- a/src/test/regress/expected/enum.out +++ b/src/test/regress/expected/enum.out @@ -581,19 +581,60 @@ ERROR: enum label "green" already exists -- 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 +ALTER TYPE bogus ADD VALUE 'new'; +SAVEPOINT x; +SELECT 'new'::bogus; -- unsafe +ERROR: unsafe use of new value "new" of enum type bogus +LINE 1: SELECT 'new'::bogus; + ^ +HINT: New enum values must be committed before they can be used. +ROLLBACK TO x; +SELECT enum_first(null::bogus); -- safe + enum_first +------------ + good +(1 row) + +SELECT enum_last(null::bogus); -- unsafe +ERROR: unsafe use of new value "new" of enum type bogus +HINT: New enum values must be committed before they can be used. +ROLLBACK TO x; +SELECT enum_range(null::bogus); -- unsafe +ERROR: unsafe use of new value "new" of enum type bogus +HINT: New enum values must be committed before they can be used. +ROLLBACK TO x; COMMIT; +SELECT 'new'::bogus; -- now safe + bogus +------- + new +(1 row) + +SELECT enumlabel, enumsortorder +FROM pg_enum +WHERE enumtypid = 'bogus'::regtype +ORDER BY 2; + enumlabel | enumsortorder +-----------+--------------- + good | 1 + new | 2 +(2 rows) + -- check that we recognize the case where the enum already existed but was --- modified in the current txn +-- modified in the current txn; this should not be considered safe BEGIN; ALTER TYPE bogus RENAME TO bogon; ALTER TYPE bogon ADD VALUE 'bad'; -ERROR: ALTER TYPE ... ADD cannot run inside a transaction block +SELECT 'bad'::bogon; +ERROR: unsafe use of new value "bad" of enum type bogon +LINE 1: SELECT 'bad'::bogon; + ^ +HINT: New enum values must be committed before they can be used. ROLLBACK; --- but ALTER TYPE RENAME VALUE is safe in a transaction +-- but a renamed value is safe to use later in same transaction BEGIN; ALTER TYPE bogus RENAME VALUE 'good' to 'bad'; SELECT 'bad'::bogus; @@ -604,12 +645,27 @@ SELECT 'bad'::bogus; ROLLBACK; DROP TYPE bogus; --- check that we *can* add new values to existing enums in a transaction, --- if the type is new as well +-- check that values created during CREATE TYPE can be used in any case +BEGIN; +CREATE TYPE bogus AS ENUM('good','bad','ugly'); +ALTER TYPE bogus RENAME TO bogon; +select enum_range(null::bogon); + enum_range +----------------- + {good,bad,ugly} +(1 row) + +ROLLBACK; +-- 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(); -ALTER TYPE bogus ADD VALUE 'good'; -ALTER TYPE bogus ADD VALUE 'ugly'; +CREATE TYPE bogus AS ENUM('good'); +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 7b68b2fe376..6affd0d1ebe 100644 --- a/src/test/regress/sql/enum.sql +++ b/src/test/regress/sql/enum.sql @@ -273,19 +273,34 @@ ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green'; -- 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'; +ALTER TYPE bogus ADD VALUE 'new'; +SAVEPOINT x; +SELECT 'new'::bogus; -- unsafe +ROLLBACK TO x; +SELECT enum_first(null::bogus); -- safe +SELECT enum_last(null::bogus); -- unsafe +ROLLBACK TO x; +SELECT enum_range(null::bogus); -- unsafe +ROLLBACK TO x; COMMIT; +SELECT 'new'::bogus; -- now safe +SELECT enumlabel, enumsortorder +FROM pg_enum +WHERE enumtypid = 'bogus'::regtype +ORDER BY 2; -- check that we recognize the case where the enum already existed but was --- modified in the current txn +-- modified in the current txn; this should not be considered safe BEGIN; ALTER TYPE bogus RENAME TO bogon; ALTER TYPE bogon ADD VALUE 'bad'; +SELECT 'bad'::bogon; ROLLBACK; --- but ALTER TYPE RENAME VALUE is safe in a transaction +-- but a renamed value is safe to use later in same transaction BEGIN; ALTER TYPE bogus RENAME VALUE 'good' to 'bad'; SELECT 'bad'::bogus; @@ -293,12 +308,21 @@ ROLLBACK; DROP TYPE bogus; --- check that we *can* add new values to existing enums in a transaction, --- if the type is new as well +-- check that values created during CREATE TYPE can be used in any case +BEGIN; +CREATE TYPE bogus AS ENUM('good','bad','ugly'); +ALTER TYPE bogus RENAME TO bogon; +select enum_range(null::bogon); +ROLLBACK; + +-- 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(); -ALTER TYPE bogus ADD VALUE 'good'; -ALTER TYPE bogus ADD VALUE 'ugly'; +CREATE TYPE bogus AS ENUM('good'); +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; -- -- 2.17.1 (Apple Git-112)