8000 Use a blacklist to distinguish original from add-on enum values. · dirbacke/postgres@1635e80 · GitHub
[go: up one dir, main page]
Skip to content

Commit 1635e80

Browse files
tglsfdctglsfdc
committed
Use a blacklist to distinguish original from add-on enum values.
Commit 15bc038 allowed ALTER TYPE ADD VALUE to be executed inside transaction blocks, by disallowing the use of the added value later in the same transaction, except under limited circumstances. However, the test for "limited circumstances" was heuristic and could reject references to enum values that were created during CREATE TYPE AS ENUM, not just later. This breaks the use-case of restoring pg_dump scripts in a single transaction, as reported in bug #14825 from Balazs Szilfai. We can improve this by keeping a "blacklist" table of enum value OIDs created by ALTER TYPE ADD VALUE during the current transaction. Any visible-but-uncommitted value whose OID is not in the blacklist must have been created by CREATE TYPE AS ENUM, and can be used safely because it could not have a lifespan shorter than its parent enum type. This change also removes the restriction that a renamed enum value can't be used before being committed (unless it was on the blacklist). Andrew Dunstan, with cosmetic improvements by me. Back-patch to v10. Discussion: https://postgr.es/m/20170922185904.1448.16585@wrigleys.postgresql.org
1 parent 15a8010 commit 1635e80

File tree

7 files changed

+113
-2
lines changed

7 files changed

+113
-2
lines changed

‎doc/src/sgml/ref/alter_type.sgml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,8 +294,6 @@ ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME VALUE <repla
294294
an enum type) is executed inside a transaction block, the new value cannot
295295
be used until after the transaction has been committed, except in the case
296296
that the enum type itself was created earlier in the same transaction.
297-
Likewise, when a pre-existing enum value is renamed, the transaction must
298-
be committed before the renamed value can be used.
299297
</para>
300298

301299
<para>

‎src/backend/access/transam/xact.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "access/xlogutils.h"
3333
#include "catalog/catalog.h"
3434
#include "catalog/namespace.h"
35+
#include "catalog/pg_enum.h"
3536
#include "catalog/storage.h"
3637
#include "commands/async.h"
3738
#include "commands/tablecmds.h"
@@ -2128,6 +2129,7 @@ CommitTransaction(void)
21282129
AtCommit_Notify();
21292130
AtEOXact_GUC(true, 1);
21302131
AtEOXact_SPI(true);
2132+
AtEOXact_Enum();
21312133
AtEOXact_on_commit_actions(true);
21322134
AtEOXact_Namespace(true, is_parallel_worker);
21332135
AtEOXact_SMgr();
@@ -2406,6 +2408,7 @@ PrepareTransaction(void)
24062408
/* PREPARE acts the same as COMMIT as far as GUC is concerned */
24072409
AtEOXact_GUC(true, 1);
24082410
AtEOXact_SPI(true);
2411+
AtEOXact_Enum();
24092412
AtEOXact_on_commit_actions(true);
24102413
AtEOXact_Namespace(true, false);
24112414
AtEOXact_SMgr();
@@ -2608,6 +2611,7 @@ AbortTransaction(void)
26082611

26092612
AtEOXact_GUC(false, 1);
26102613
AtEOXact_SPI(false);
2614+
AtEOXact_Enum();
26112615
AtEOXact_on_commit_actions(false);
26122616
AtEOXact_Namespace(false, is_parallel_worker);
26132617
AtEOXact_SMgr();

‎src/backend/catalog/pg_enum.c

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,26 @@
2828
#include "utils/builtins.h"
2929
#include "utils/catcache.h"
3030
#include "utils/fmgroids.h"
31+
#include "utils/hsearch.h"
32+
#include "utils/memutils.h"
3133
#include "utils/syscache.h"
3234
#include "utils/tqual.h"
3335

3436

3537
/* Potentially set by pg_upgrade_support functions */
3638
Oid binary_upgrade_next_pg_enum_oid = InvalidOid;
3739

40+
/*
41+
* Hash table of enum value OIDs created during the current transaction by
42+
* AddEnumLabel. We disallow using these values until the transaction is
43+
* committed; otherwise, they might get into indexes where we can't clean
44+
* them up, and then if the transaction rolls back we have a broken index.
45+
* (See comments for check_safe_enum_use() in enum.c.) Values created by
46+
* EnumValuesCreate are *not* blacklisted; we assume those are created during
47+
* CREATE TYPE, so they can't go away unless the enum type itself does.
48+
*/
49+
static HTAB *enum_blacklist = NULL;
50+
3851
static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
3952
static int sort_order_cmp(const void *p1, const void *p2);
4053

@@ -460,6 +473,24 @@ AddEnumLabel(Oid enumTypeOid,
460473
heap_freetuple(enum_tup);
461474

462475
heap_close(pg_enum, RowExclusiveLock);
476+
477+
/* Set up the blacklist hash if not already done in this transaction */
478+
if (enum_blacklist == NULL)
479+
{
480+
HASHCTL hash_ctl;
481+
482+
memset(&hash_ctl, 0, sizeof(hash_ctl));
483+
hash_ctl.keysize = sizeof(Oid);
484+
hash_ctl.entrysize = sizeof(Oid);
485+
hash_ctl.hcxt = TopTransactionContext;
486+
enum_blacklist = hash_create("Enum value blacklist",
487+
32,
488+
&hash_ctl,
489+
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
490+
}
491+
492+
/* Add the new value to the blacklist */
493+
(void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL);
463494
}
464495

465496

@@ -547,6 +578,39 @@ RenameEnumLabel(Oid enumTypeOid,
547578
}
548579

549580

581+
/*
582+
* Test if the given enum value is on the blacklist
583+
*/
584+
bool
585+
EnumBlacklisted(Oid enum_id)
586+
{
587+
bool found;
588+
589+
/* If we've made no blacklist table, all values are safe */
590+
if (enum_blacklist == NULL)
591+
return false;
592+
593+
/* Else, is it in the table? */
594+
(void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found);
595+
return found;
596+
}
597+
598+
599+
/*
600+
* Clean up enum stuff after end of top-level transaction.
601+
*/
602+
void
603+
AtEOXact_Enum(void)
604+
{
605+
/*
606+
* Reset the blacklist table, as all our enum values are now committed.
607+
* The memory will go away automatically when TopTransactionContext is
608+
* freed; it's sufficient to clear our pointer.
609+
*/
610+
enum_blacklist = NULL;
611+
}
612+
613+
550614
/*
551615
* RenumberEnumType
552616
* Renumber existing enum elements to have sort positions 1..n.

‎src/backend/utils/adt/enum.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,15 @@ check_safe_enum_use(HeapTuple enumval_tup)
7676
TransactionIdDidCommit(xmin))
7777
return;
7878

79+
/*
80+
* Check if the enum value is blacklisted. If not, it's safe, because it
81+
* was made during CREATE TYPE AS ENUM and can't be shorter-lived than its
82+
* owning type. (This'd also be false for values made by other
83+
* transactions; but the previous tests should have handled all of those.)
84+
*/
85+
if (!EnumBlacklisted(HeapTupleGetOid(enumval_tup)))
86+
return;
87+
7988
/* It is a new enum value, so check to see if the whole enum is new */
8089
en = (Form_pg_enum) GETSTRUCT(enumval_tup);
8190
enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));

‎src/include/catalog/pg_enum.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,5 +69,7 @@ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
6969
bool skipIfExists);
7070
extern void RenameEnumLabel(Oid enumTypeOid,
7171
const char *oldVal, const char *newVal);
72+
extern bool EnumBlacklisted(Oid enum_id);
73+
extern void AtEOXact_Enum(void);
7274

7375
#endif /* PG_ENUM_H */

‎src/test/regress/expected/enum.out

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -633,8 +633,29 @@ ERROR: unsafe use of new value "bad" of enum type bogon
633633
LINE 1: SELECT 'bad'::bogon;
634634
^
635635
HINT: New enum values must be committed before they can be used.
636+
ROLLBACK;
637+
-- but a renamed value is safe to use later in same transaction
638+
BEGIN;
639+
ALTER TYPE bogus RENAME VALUE 'good' to 'bad';
640+
SELECT 'bad'::bogus;
641+
bogus
642+
-------
643+
bad
644+
(1 row)
645+
636646
ROLLBACK;
637647
DROP TYPE bogus;
648+
-- check that values created during CREATE TYPE can be used in any case
649+
BEGIN;
650+
CREATE TYPE bogus AS ENUM('good','bad','ugly');
651+
ALTER TYPE bogus RENAME TO bogon;
652+
select enum_range(null::bogon);
653+
enum_range
654+
-----------------
655+
{good,bad,ugly}
656+
(1 row)
657+
658+
ROLLBACK;
638659
-- check that we can add new values to existing enums in a transaction
639660
-- and use them, if the type is new as well
640661
BEGIN;

‎src/test/regress/sql/enum.sql

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,21 @@ ALTER TYPE bogon ADD VALUE 'bad';
300300
SELECT 'bad'::bogon;
301301
ROLLBACK;
302302

303+
-- but a renamed value is safe to use later in same transaction
304+
BEGIN;
305+
ALTER TYPE bogus RENAME VALUE 'good' to 'bad';
306+
SELECT 'bad'::bogus;
307+
ROLLBACK;
308+
303309
DROP TYPE bogus;
304310

311+
-- check that values created during CREATE TYPE can be used in any case
312+
BEGIN;
313+
CREATE TYPE bogus AS ENUM('good','bad','ugly');
314+
ALTER TYPE bogus RENAME TO bogon;
315+
select enum_range(null::bogon);
316+
ROLLBACK;
317+
305318
-- check that we can add new values to existing enums in a transaction
306319
-- and use them, if the type is new as well
307320
BEGIN;

0 commit comments

Comments
 (0)
0