summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: d37e0d0)
raw | patch | inline | side by side (parent: d37e0d0)
| author | Tom Lane <tgl@sss.pgh.pa.us> | |
| Sun, 24 Mar 2024 18:30:29 +0000 (14:30 -0400) | ||
| committer | Tom Lane <tgl@sss.pgh.pa.us> | |
| Sun, 24 Mar 2024 18:30:29 +0000 (14:30 -0400) |
Up to now we've rejected cases like
BEGIN;
CREATE TYPE rainbow AS ENUM ();
ALTER TYPE rainbow ADD VALUE 'red';
-- use the value 'red', perhaps in a constraint or index
COMMIT;
The concern is that the uncommitted enum value 'red' might get into
an index and then break the index if we roll back the ALTER ADD.
If the ALTER is in the same transaction as the CREATE then it's really
perfectly safe, but we weren't taking the trouble to identify that.
pg_dump in binary-upgrade mode will emit enum definitions that look
like the above, which up to now didn't fall foul of the unsafe-usage
check because we processed each restore command as a separate
transaction. However an upcoming patch proposes to bundle the restore
commands into large transactions to reduce XID consumption during
pg_upgrade, and that makes this behavior a problem.
To fix, remember the OIDs of enum types created in the current
transaction, and allow use of enum values that are added to one later
in the same transaction. To do this fully correctly in the presence
of subtransactions, we'd have to track subtransaction nesting level of
the CREATE and do maintenance work at every subsequent subtransaction
exit. That seems expensive, and we don't need it to satisfy pg_dump's
usage. Hence, apply the additional optimization only when the CREATE
and ALTER are at outermost transaction level.
Patch by me, reviewed by Andrew Dunstan
Discussion: https://postgr.es/m/1548468.1711220438@sss.pgh.pa.us
BEGIN;
CREATE TYPE rainbow AS ENUM ();
ALTER TYPE rainbow ADD VALUE 'red';
-- use the value 'red', perhaps in a constraint or index
COMMIT;
The concern is that the uncommitted enum value 'red' might get into
an index and then break the index if we roll back the ALTER ADD.
If the ALTER is in the same transaction as the CREATE then it's really
perfectly safe, but we weren't taking the trouble to identify that.
pg_dump in binary-upgrade mode will emit enum definitions that look
like the above, which up to now didn't fall foul of the unsafe-usage
check because we processed each restore command as a separate
transaction. However an upcoming patch proposes to bundle the restore
commands into large transactions to reduce XID consumption during
pg_upgrade, and that makes this behavior a problem.
To fix, remember the OIDs of enum types created in the current
transaction, and allow use of enum values that are added to one later
in the same transaction. To do this fully correctly in the presence
of subtransactions, we'd have to track subtransaction nesting level of
the CREATE and do maintenance work at every subsequent subtransaction
exit. That seems expensive, and we don't need it to satisfy pg_dump's
usage. Hence, apply the additional optimization only when the CREATE
and ALTER are at outermost transaction level.
Patch by me, reviewed by Andrew Dunstan
Discussion: https://postgr.es/m/1548468.1711220438@sss.pgh.pa.us
index d2fe5ca2c85634be61831caeb7161bb3151ea4e2..54cededac1bf3e48750a1ce85c929edea480efc7 100644 (file)
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
+ * We keep two transaction-lifespan hash tables, one containing the OIDs
+ * of enum types made in the current transaction, and one containing the
+ * OIDs of enum values created during the current transaction by
+ * AddEnumLabel (but only if their enum type is not in the first hash).
+ *
+ * We disallow using enum values in the second hash 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* entered into the table; we assume those are
* created during CREATE TYPE, so they can't go away unless the enum type
* itself does.
+ *
+ * The motivation for treating enum values as safe if their type OID is
+ * in the first hash is to allow CREATE TYPE AS ENUM; ALTER TYPE ADD VALUE;
+ * followed by a use of the value in the same transaction. This pattern
+ * is really just as safe as creating the value during CREATE TYPE.
+ * We need to support this because pg_dump in binary upgrade mode produces
+ * commands like that. But currently we only support it when the commands
+ * are at the outermost transaction level, which is as much as we need for
+ * pg_dump. We could track subtransaction nesting of the commands to
+ * analyze things more precisely, but for now we don't bother.
*/
-static HTAB *uncommitted_enums = NULL;
+static HTAB *uncommitted_enum_types = NULL;
+static HTAB *uncommitted_enum_values = NULL;
+static void init_uncommitted_enum_types(void);
+static void init_uncommitted_enum_values(void);
+static bool EnumTypeUncommitted(Oid typ_id);
static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
static int sort_order_cmp(const void *p1, const void *p2);
* Create an entry in pg_enum for each of the supplied enum values.
*
* vals is a list of String values.
+ *
+ * We assume that this is called only by CREATE TYPE AS ENUM, and that it
+ * will be called even if the vals list is empty. So we can enter the
+ * enum type's OID into uncommitted_enum_types here, rather than needing
+ * another entry point to do it.
*/
void
EnumValuesCreate(Oid enumTypeOid, List *vals)
CatalogIndexState indstate;
TupleTableSlot **slot;
+ /*
+ * Remember the type OID as being made in the current transaction, but not
+ * if we're in a subtransaction. (We could remember the OID anyway, in
+ * case a subsequent ALTER ADD VALUE occurs at outer level. But that
+ * usage pattern seems unlikely enough that we'd probably just be wasting
+ * hashtable maintenance effort.)
+ */
+ if (GetCurrentTransactionNestLevel() == 1)
+ {
+ if (uncommitted_enum_types == NULL)
+ init_uncommitted_enum_types();
+ (void) hash_search(uncommitted_enum_types, &enumTypeOid,
+ HASH_ENTER, NULL);
+ }
+
num_elems = list_length(vals);
/*
}
/*
- * Initialize the uncommitted enum table for this transaction.
+ * Initialize the uncommitted enum types table for this transaction.
+ */
+static void
+init_uncommitted_enum_types(void)
+{
+ HASHCTL hash_ctl;
+
+ hash_ctl.keysize = sizeof(Oid);
+ hash_ctl.entrysize = sizeof(Oid);
+ hash_ctl.hcxt = TopTransactionContext;
+ uncommitted_enum_types = hash_create("Uncommitted enum types",
+ 32,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+}
+
+/*
+ * Initialize the uncommitted enum values table for this transaction.
*/
static void
-init_uncommitted_enums(void)
+init_uncommitted_enum_values(void)
{
HASHCTL hash_ctl;
hash_ctl.keysize = sizeof(Oid);
hash_ctl.entrysize = sizeof(Oid);
hash_ctl.hcxt = TopTransactionContext;
- uncommitted_enums = hash_create("Uncommitted enums",
- 32,
- &hash_ctl,
- HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+ uncommitted_enum_values = hash_create("Uncommitted enum values",
+ 32,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
}
/*
table_close(pg_enum, RowExclusiveLock);
- /* Set up the uncommitted enum table if not already done in this tx */
- if (uncommitted_enums == NULL)
- init_uncommitted_enums();
+ /*
+ * If the enum type itself is uncommitted, we need not enter the new enum
+ * value into uncommitted_enum_values, because the type won't survive if
+ * the value doesn't. (This is basically the same reasoning as for values
+ * made directly by CREATE TYPE AS ENUM.) However, apply this rule only
+ * when we are not inside a subtransaction; if we're more deeply nested
+ * than the CREATE TYPE then the conclusion doesn't hold. We could expend
+ * more effort to track the subtransaction level of CREATE TYPE, but for
+ * now we're only concerned about making the world safe for pg_dump in
+ * binary upgrade mode, and that won't use subtransactions.
+ */
+ if (GetCurrentTransactionNestLevel() == 1 &&
+ EnumTypeUncommitted(enumTypeOid))
+ return;
+
+ /* Set up the uncommitted values table if not already done in this tx */
+ if (uncommitted_enum_values == NULL)
+ init_uncommitted_enum_values();
/* Add the new value to the table */
- (void) hash_search(uncommitted_enums, &newOid, HASH_ENTER, NULL);
+ (void) hash_search(uncommitted_enum_values, &newOid, HASH_ENTER, NULL);
}
/*
- * Test if the given enum value is in the table of uncommitted enums.
+ * Test if the given type OID is in the table of uncommitted enum types.
+ */
+static bool
+EnumTypeUncommitted(Oid typ_id)
+{
+ bool found;
+
+ /* If we've made no uncommitted types table, it's not in the table */
+ if (uncommitted_enum_types == NULL)
+ return false;
+
+ /* Else, is it in the table? */
+ (void) hash_search(uncommitted_enum_types, &typ_id, HASH_FIND, &found);
+ return found;
+}
+
+
+/*
+ * Test if the given enum value is in the table of uncommitted enum values.
*/
bool
EnumUncommitted(Oid enum_id)
{
bool found;
- /* If we've made no uncommitted table, all values are safe */
- if (uncommitted_enums == NULL)
+ /* If we've made no uncommitted values table, it's not in the table */
+ if (uncommitted_enum_values == NULL)
return false;
/* Else, is it in the table? */
- (void) hash_search(uncommitted_enums, &enum_id, HASH_FIND, &found);
+ (void) hash_search(uncommitted_enum_values, &enum_id, HASH_FIND, &found);
return found;
}
AtEOXact_Enum(void)
{
/*
- * Reset the uncommitted 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.
+ * Reset the uncommitted tables, as all our tuples are now committed. The
+ * memory will go away automatically when TopTransactionContext is freed;
+ * it's sufficient to clear our pointers.
*/
- uncommitted_enums = NULL;
+ uncommitted_enum_types = NULL;
+ uncommitted_enum_values = NULL;
}
Size
EstimateUncommittedEnumsSpace(void)
{
- size_t entries;
+ size_t entries = 0;
- if (uncommitted_enums)
- entries = hash_get_num_entries(uncommitted_enums);
- else
- entries = 0;
+ if (uncommitted_enum_types)
+ entries += hash_get_num_entries(uncommitted_enum_types);
+ if (uncommitted_enum_values)
+ entries += hash_get_num_entries(uncommitted_enum_values);
- /* Add one for the terminator. */
- return sizeof(Oid) * (entries + 1);
+ /* Add two for the terminators. */
+ return sizeof(Oid) * (entries + 2);
}
void
Oid *serialized = (Oid *) space;
/*
- * Make sure the hash table hasn't changed in size since the caller
+ * Make sure the hash tables haven't changed in size since the caller
* reserved the space.
*/
Assert(size == EstimateUncommittedEnumsSpace());
- /* Write out all the values from the hash table, if there is one. */
- if (uncommitted_enums)
+ /* Write out all the OIDs from the types hash table, if there is one. */
+ if (uncommitted_enum_types)
{
HASH_SEQ_STATUS status;
Oid *value;
- hash_seq_init(&status, uncommitted_enums);
+ hash_seq_init(&status, uncommitted_enum_types);
while ((value = (Oid *) hash_seq_search(&status)))
*serialized++ = *value;
}
/* Write out the terminator. */
- *serialized = InvalidOid;
+ *serialized++ = InvalidOid;
+
+ /* Write out all the OIDs from the values hash table, if there is one. */
+ if (uncommitted_enum_values)
+ {
+ HASH_SEQ_STATUS status;
+ Oid *value;
+
+ hash_seq_init(&status, uncommitted_enum_values);
+ 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);
+ Assert((char *) serialized == ((char *) space) + size);
}
void
{
Oid *serialized = (Oid *) space;
- Assert(!uncommitted_enums);
+ Assert(!uncommitted_enum_types);
+ Assert(!uncommitted_enum_values);
/*
- * 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 either list is empty then don't even bother to create that hash
+ * table. This is the common case, since most transactions don't create
+ * or alter enums.
*/
- if (!OidIsValid(*serialized))
- return;
-
- /* Read all the values into a new hash table. */
- init_uncommitted_enums();
- do
+ if (OidIsValid(*serialized))
{
- hash_search(uncommitted_enums, serialized++, HASH_ENTER, NULL);
- } while (OidIsValid(*serialized));
+ /* Read all the types into a new hash table. */
+ init_uncommitted_enum_types();
+ do
+ {
+ (void) hash_search(uncommitted_enum_types, serialized++,
+ HASH_ENTER, NULL);
+ } while (OidIsValid(*serialized));
+ }
+ serialized++;
+ if (OidIsValid(*serialized))
+ {
+ /* Read all the values into a new hash table. */
+ init_uncommitted_enum_values();
+ do
+ {
+ (void) hash_search(uncommitted_enum_values, serialized++,
+ HASH_ENTER, NULL);
+ } while (OidIsValid(*serialized));
+ }
}
index f649ff2c564c7c0a06980fc3779f85755062e7c5..814c7fb4e3e2160f6f71eb8609db1f303472e353 100644 (file)
* 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.)
+ * Values added by ALTER TYPE ADD VALUE are also allowed if the enum type
+ * is known to have been created earlier in the same transaction. (Note that
+ * we have to track that explicitly; comparing tuple xmins is insufficient,
+ * because the type tuple might have been updated in the current transaction.
+ * Subtransactions also create hazards to be accounted for; currently,
+ * pg_enum.c only handles ADD VALUE at the outermost transaction level.)
*
* This function needs to be called (directly or indirectly) in any of the
* functions below that could return an enum value to SQL operations.
return;
/*
- * Check if the enum value is uncommitted. 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.)
+ * Check if the enum value is listed as uncommitted. If not, it's safe,
+ * because it 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 (!EnumUncommitted(en->oid))
return;
index 01159688e57cad99f2c4d44617661b30ffbd14a4..1d09c208bc9030ff23c6a30fcea964a7d7af13cc 100644 (file)
(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
+-- we must allow this usage to support pg_dump in binary upgrade mode
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.
+select enum_range(null::bogon);
+ enum_range
+-----------------
+ {good,bad,ugly}
+(1 row)
+
ROLLBACK;
--
-- Cleanup
index 93171379f25fc91037b8d8e3aecd087e1d7a8431..ecc4878a6782a32b2fd9eddee771a0bf703a6c7e 100644 (file)
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
+-- we must allow this usage to support pg_dump in binary upgrade mode
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
+select enum_range(null::bogon);
ROLLBACK;
--