From 212fab9926b2f0f04b0187568e7124b70e8deee5 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 9 Oct 2018 12:51:01 +1300 Subject: [PATCH] Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux). Originally committed as 15bc038f (plus some follow-ups), this was reverted in 28e07270 due to a problem discovered in parallel workers. This new version corrects that problem by sending the list of uncommitted enum values to parallel workers. Here follows the original commit message describing the change: To prevent possibly breaking indexes on enum columns, we must keep uncommitted enum values from getting stored in tables, unless we can be sure that any such column is new in the current transaction. Formerly, we enforced this by disallowing ALTER TYPE ... ADD VALUE from being executed at all in a transaction block, unless the target enum type had been created in the current transaction. This patch removes that restriction, and instead insists that an uncommitted enum value can't be referenced unless it belongs to an enum type created in the same transaction as the value. Per discussion, this should be a bit less onerous. It does require each function that could possibly return a new enum value to SQL operations to check this restriction, but there aren't so many of those that this seems unmaintainable. Author: Andrew Dunstan and Tom Lane, with parallel query fix by Thomas Munro Reviewed-by: Tom Lane Discussion: https://postgr.es/m/CAEepm%3D0Ei7g6PaNTbcmAh9tCRahQrk%3Dr5ZWLD-jr7hXweYX3yg%40mail.gmail.com Discussion: https://postgr.es/m/4075.1459088427%40sss.pgh.pa.us --- doc/src/sgml/ref/alter_type.sgml | 5 +- src/backend/access/transam/parallel.c | 20 +++- src/backend/access/transam/xact.c | 4 + src/backend/catalog/pg_enum.c | 139 ++++++++++++++++++++++++++ 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 | 5 + src/include/commands/typecmds.h | 2 +- src/test/regress/expected/enum.out | 78 +++++++++++++-- src/test/regress/sql/enum.sql | 42 ++++++-- 12 files changed, 367 insertions(+), 50 deletions(-) diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml index 9127dfd88d..67be1dd568 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/parallel.c b/src/backend/access/transam/parallel.c index 911da776e8..84197192ec 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -19,6 +19,7 @@ #include "access/session.h" #include "access/xact.h" #include "access/xlog.h" +#include "catalog/pg_enum.h" #include "catalog/index.h" #include "catalog/namespace.h" #include "commands/async.h" @@ -71,6 +72,7 @@ #define PARALLEL_KEY_SESSION_DSM UINT64CONST(0xFFFFFFFFFFFF000A) #define PARALLEL_KEY_REINDEX_STATE UINT64CONST(0xFFFFFFFFFFFF000B) #define PARALLEL_KEY_RELMAPPER_STATE UINT64CONST(0xFFFFFFFFFFFF000C) +#define PARALLEL_KEY_ENUMBLACKLIST UINT64CONST(0xFFFFFFFFFFFF000D) /* Fixed-size parallel state. */ typedef struct FixedParallelState @@ -210,6 +212,7 @@ InitializeParallelDSM(ParallelContext *pcxt) Size tstatelen = 0; Size reindexlen = 0; Size relmapperlen = 0; + Size enumblacklistlen = 0; Size segsize = 0; int i; FixedParallelState *fps; @@ -263,8 +266,10 @@ InitializeParallelDSM(ParallelContext *pcxt) shm_toc_estimate_chunk(&pcxt->estimator, reindexlen); relmapperlen = EstimateRelationMapSpace(); shm_toc_estimate_chunk(&pcxt->estimator, relmapperlen); + enumblacklistlen = EstimateEnumBlacklistSpace(); + shm_toc_estimate_chunk(&pcxt->estimator, enumblacklistlen); /* If you add more chunks here, you probably need to add keys. */ - shm_toc_estimate_keys(&pcxt->estimator, 9); + shm_toc_estimate_keys(&pcxt->estimator, 10); /* Estimate space need for error queues. */ StaticAssertStmt(BUFFERALIGN(PARALLEL_ERROR_QUEUE_SIZE) == @@ -340,6 +345,7 @@ InitializeParallelDSM(ParallelContext *pcxt) char *error_queue_space; char *session_dsm_handle_space; char *entrypointstate; + char *enumblacklistspace; Size lnamelen; /* Serialize shared libraries we have loaded. */ @@ -389,6 +395,12 @@ InitializeParallelDSM(ParallelContext *pcxt) shm_toc_insert(pcxt->toc, PARALLEL_KEY_RELMAPPER_STATE, relmapperspace); + /* Serialize enum blacklist state. */ + enumblacklistspace = shm_toc_allocate(pcxt->toc, enumblacklistlen); + SerializeEnumBlacklist(enumblacklistspace, enumblacklistlen); + shm_toc_insert(pcxt->toc, PARALLEL_KEY_ENUMBLACKLIST, + enumblacklistspace); + /* Allocate space for worker information. */ pcxt->worker = palloc0(sizeof(ParallelWorkerInfo) * pcxt->nworkers); @@ -1222,6 +1234,7 @@ ParallelWorkerMain(Datum main_arg) char *tstatespace; char *reindexspace; char *relmapperspace; + char *enumblacklistspace; StringInfoData msgbuf; char *session_dsm_handle_space; @@ -1408,6 +1421,11 @@ ParallelWorkerMain(Datum main_arg) relmapperspace = shm_toc_lookup(toc, PARALLEL_KEY_RELMAPPER_STATE, false); RestoreRelationMap(relmapperspace); + /* Restore enum blacklist. */ + enumblacklistspace = shm_toc_lookup(toc, PARALLEL_KEY_ENUMBLACKLIST, + false); + RestoreEnumBlacklist(enumblacklistspace); + /* * We've initialized all of our state now; nothing should change * hereafter. diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 59e021f887..6cd00d9aaa 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" @@ -2135,6 +2136,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(); @@ -2413,6 +2415,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(); @@ -2615,6 +2618,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 fde361f367..ece65587bb 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); @@ -168,6 +181,23 @@ EnumValuesDelete(Oid enumTypeOid) heap_close(pg_enum, RowExclusiveLock); } +/* + * Initialize the enum blacklist for this transaction. + */ +static void +init_enum_blacklist(void) +{ + 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); +} /* * AddEnumLabel @@ -460,6 +490,13 @@ 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) + init_enum_blacklist(); + + /* Add the new value to the blacklist */ + (void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL); } @@ -547,6 +584,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. @@ -620,3 +690,72 @@ sort_order_cmp(const void *p1, const void *p2) else return 0; } + +Size +EstimateEnumBlacklistSpace(void) +{ + size_t entries; + + if (enum_blacklist) + entries = hash_get_num_entries(enum_blacklist); + else + entries = 0; + + /* Add one for the terminator. */ + return sizeof(Oid) * (entries + 1); +} + +void +SerializeEnumBlacklist(void *space, Size size) +{ + Oid *serialized = (Oid *) space; + + /* + * Make sure the hash table hasn't changed in size since the caller + * reserved the space. + */ + Assert(size == EstimateEnumBlacklistSpace()); + + /* Write out all the values from the hash table, if there is one. */ + if (enum_blacklist) + { + HASH_SEQ_STATUS status; + Oid *value; + + hash_seq_init(&status, enum_blacklist); + while ((value = (Oid *) hash_seq_search(&status))) + *serialized++ = *value; + } + + /* Write out the terminator. */ + *serialized = InvalidOid; + + /* + * Make sure the amount of space we actually used matches what was + * estimated. + */ + Assert((char *) (serialized + 1) == ((char *) space) + size); +} + +void +RestoreEnumBlacklist(void *space) +{ + Oid *serialized = (Oid *) space; + + Assert(!enum_blacklist); + + /* + * As a special case, if the list is empty then don't even bother to + * create the hash table. This is the usual case, since enum alteration + * is expected to be rare. + */ + if (!OidIsValid(*serialized)) + return; + + /* Read all the values into a new hash table. */ + init_enum_blacklist(); + do + { + hash_search(enum_blacklist, serialized++, HASH_ENTER, NULL); + } while (OidIsValid(*serialized)); +} diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index b018585aef..3271962a7a 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 b5804f64ad..898091c45f 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 562cd5c555..01edccced2 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 29efb3f6ef..788f88129b 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 e6fd41e5b2..474877749b 100644 --- a/src/include/catalog/pg_enum.h +++ b/src/include/catalog/pg_enum.h @@ -52,5 +52,10 @@ 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 Size EstimateEnumBlacklistSpace(void); +extern void SerializeEnumBlacklist(void *space, Size size); +extern void RestoreEnumBlacklist(void *space); +extern void AtEOXact_Enum(void); #endif /* PG_ENUM_H */ diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h index a04f3405de..ac4ce50ef6 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 a0b81608a1..4f839ce027 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(); -ALTER TYPE bogus ADD VALUE 'good'; -ALTER TYPE bogus ADD VALUE 'ugly'; +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('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 7b68b2fe37..6affd0d1eb 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(); -ALTER TYPE bogus ADD VALUE 'good'; -ALTER TYPE bogus ADD VALUE 'ugly'; +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('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; --