Skip to content

Commit 212fab9

Browse files
macdicemacdice
committed
Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux).
Originally committed as 15bc038 (plus some follow-ups), this was reverted in 28e0727 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
1 parent 7767aad commit 212fab9

File tree

12 files changed

+367
-50
lines changed

12 files changed

+367
-50
lines changed

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,8 +290,9 @@ ALTER TYPE <replaceable class="parameter">name</replaceable> RENAME VALUE <repla
290290
<title>Notes</title>
291291

292292
<para>
293-
<command>ALTER TYPE ... ADD VALUE</command> (the form that adds a new value to an
294-
enum type) cannot be executed inside a transaction block.
293+
If <command>ALTER TYPE ... ADD VALUE</command> (the form that adds a new
294+
value to an enum type) is executed inside a transaction block, the new
295+
value cannot be used until after the transaction has been committed.
295296
</para>
296297

297298
<para>

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "access/session.h"
2020
#include "access/xact.h"
2121
#include "access/xlog.h"
22+
#include "catalog/pg_enum.h"
2223
#include "catalog/index.h"
2324
#include "catalog/namespace.h"
2425
#include "commands/async.h"
@@ -71,6 +72,7 @@
7172
#define PARALLEL_KEY_SESSION_DSM UINT64CONST(0xFFFFFFFFFFFF000A)
7273
#define PARALLEL_KEY_REINDEX_STATE UINT64CONST(0xFFFFFFFFFFFF000B)
7374
#define PARALLEL_KEY_RELMAPPER_STATE UINT64CONST(0xFFFFFFFFFFFF000C)
75+
#define PARALLEL_KEY_ENUMBLACKLIST UINT64CONST(0xFFFFFFFFFFFF000D)
7476

7577
/* Fixed-size parallel state. */
7678
typedef struct FixedParallelState
@@ -210,6 +212,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
210212
Size tstatelen = 0;
211213
Size reindexlen = 0;
212214
Size relmapperlen = 0;
215+
Size enumblacklistlen = 0;
213216
Size segsize = 0;
214217
int i;
215218
FixedParallelState *fps;
@@ -263,8 +266,10 @@ InitializeParallelDSM(ParallelContext *pcxt)
263266
shm_toc_estimate_chunk(&pcxt->estimator, reindexlen);
264267
relmapperlen = EstimateRelationMapSpace();
265268
shm_toc_estimate_chunk(&pcxt->estimator, relmapperlen);
269+
enumblacklistlen = EstimateEnumBlacklistSpace();
270+
shm_toc_estimate_chunk(&pcxt->estimator, enumblacklistlen);
266271
/* If you add more chunks here, you probably need to add keys. */
267-
shm_toc_estimate_keys(&pcxt->estimator, 9);
272+
shm_toc_estimate_keys(&pcxt->estimator, 10);
268273

269274
/* Estimate space need for error queues. */
270275
StaticAssertStmt(BUFFERALIGN(PARALLEL_ERROR_QUEUE_SIZE) ==
@@ -340,6 +345,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
340345
char *error_queue_space;
341346
char *session_dsm_handle_space;
342347
char *entrypointstate;
348+
char *enumblacklistspace;
343349
Size lnamelen;
344350

345351
/* Serialize shared libraries we have loaded. */
@@ -389,6 +395,12 @@ InitializeParallelDSM(ParallelContext *pcxt)
389395
shm_toc_insert(pcxt->toc, PARALLEL_KEY_RELMAPPER_STATE,
390396
relmapperspace);
391397

398+
/* Serialize enum blacklist state. */
399+
enumblacklistspace = shm_toc_allocate(pcxt->toc, enumblacklistlen);
400+
SerializeEnumBlacklist(enumblacklistspace, enumblacklistlen);
401+
shm_toc_insert(pcxt->toc, PARALLEL_KEY_ENUMBLACKLIST,
402+
enumblacklistspace);
403+
392404
/* Allocate space for worker information. */
393405
pcxt->worker = palloc0(sizeof(ParallelWorkerInfo) * pcxt->nworkers);
394406

@@ -1222,6 +1234,7 @@ ParallelWorkerMain(Datum main_arg)
12221234
char *tstatespace;
12231235
char *reindexspace;
12241236
char *relmapperspace;
1237+
char *enumblacklistspace;
12251238
StringInfoData msgbuf;
12261239
char *session_dsm_handle_space;
12271240

@@ -1408,6 +1421,11 @@ ParallelWorkerMain(Datum main_arg)
14081421
relmapperspace = shm_toc_lookup(toc, PARALLEL_KEY_RELMAPPER_STATE, false);
14091422
RestoreRelationMap(relmapperspace);
14101423

1424+
/* Restore enum blacklist. */
1425+
enumblacklistspace = shm_toc_lookup(toc, PARALLEL_KEY_ENUMBLACKLIST,
1426+
false);
1427+
RestoreEnumBlacklist(enumblacklistspace);
1428+
14111429
/*
14121430
* We've initialized all of our state now; nothing should change
14131431
* hereafter.

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "access/xloginsert.h"
3232
#include "access/xlogutils.h"
3333
#include "catalog/namespace.h"
34+
#include "catalog/pg_enum.h"
3435
#include "catalog/storage.h"
3536
#include "commands/async.h"
3637
#include "commands/tablecmds.h"
@@ -2135,6 +2136,7 @@ CommitTransaction(void)
21352136
AtCommit_Notify();
21362137
AtEOXact_GUC(true, 1);
21372138
AtEOXact_SPI(true);
2139+
AtEOXact_Enum();
21382140
AtEOXact_on_commit_actions(true);
21392141
AtEOXact_Namespace(true, is_parallel_worker);
21402142
AtEOXact_SMgr();
@@ -2413,6 +2415,7 @@ PrepareTransaction(void)
24132415
/* PREPARE acts the same as COMMIT as far as GUC is concerned */
24142416
AtEOXact_GUC(true, 1);
24152417
AtEOXact_SPI(true);
2418+
AtEOXact_Enum();
24162419
AtEOXact_on_commit_actions(true);
24172420
AtEOXact_Namespace(true, false);
24182421
AtEOXact_SMgr();
@@ -2615,6 +2618,7 @@ AbortTransaction(void)
26152618

26162619
AtEOXact_GUC(false, 1);
26172620
AtEOXact_SPI(false);
2621+
AtEOXact_Enum();
26182622
AtEOXact_on_commit_actions(false);
26192623
AtEOXact_Namespace(false, is_parallel_worker);
26202624
AtEOXact_SMgr();

‎src/backend/catalog/pg_enum.c

Lines changed: 139 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

@@ -168,6 +181,23 @@ EnumValuesDelete(Oid enumTypeOid)
168181
heap_close(pg_enum, RowExclusiveLock);
169182
}
170183

184+
/*
185+
* Initialize the enum blacklist for this transaction.
186+
*/
187+
static void
188+
init_enum_blacklist(void)
189+
{
190+
HASHCTL hash_ctl;
191+
192+
memset(&hash_ctl, 0, sizeof(hash_ctl));
193+
hash_ctl.keysize = sizeof(Oid);
194+
hash_ctl.entrysize = sizeof(Oid);
195+
hash_ctl.hcxt = TopTransactionContext;
196+
enum_blacklist = hash_create("Enum value blacklist",
197+
32,
198+
&hash_ctl,
199+
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
200+
}
171201

172202
/*
173203
* AddEnumLabel
@@ -460,6 +490,13 @@ AddEnumLabel(Oid enumTypeOid,
460490
heap_freetuple(enum_tup);
461491

462492
heap_close(pg_enum, RowExclusiveLock);
493+
494+
/* Set up the blacklist hash if not already done in this transaction */
495+
if (enum_blacklist == NULL)
496+
init_enum_blacklist();
497+
498+
/* Add the new value to the blacklist */
499+
(void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL);
463500
}
464501

465502

@@ -547,6 +584,39 @@ RenameEnumLabel(Oid enumTypeOid,
547584
}
548585

549586

587+
/*
588+
* Test if the given enum value is on the blacklist
589+
*/
590+
bool
591+
EnumBlacklisted(Oid enum_id)
592+
{
593+
bool found;
594+
595+
/* If we've made no blacklist table, all values are safe */
596+
if (enum_blacklist == NULL)
597+
return false;
598+
599+
/* Else, is it in the table? */
600+
(void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found);
601+
return found;
602+
}
603+
604+
605+
/*
606+
* Clean up enum stuff after end of top-level transaction.
607+
*/
608+
void
609+
AtEOXact_Enum(void)
610+
{
611+
/*
612+
* Reset the blacklist table, as all our enum values are now committed.
613+
* The memory will go away automatically when TopTransactionContext is
614+
* freed; it's sufficient to clear our pointer.
615+
*/
616+
enum_blacklist = NULL;
617+
}
618+
619+
550620
/*
551621
* RenumberEnumType
552622
* Renumber existing enum elements to have sort positions 1..n.
@@ -620,3 +690,72 @@ sort_order_cmp(const void *p1, const void *p2)
620690
else
621691
return 0;
622692
}
693+
694+
Size
695+
EstimateEnumBlacklistSpace(void)
696+
{
697+
size_t entries;
698+
699+
if (enum_blacklist)
700+
entries = hash_get_num_entries(enum_blacklist);
701+
else
702+
entries = 0;
703+
704+
/* Add one for the terminator. */
705+
return sizeof(Oid) * (entries + 1);
706+
}
707+
708+
void
709+
SerializeEnumBlacklist(void *space, Size size)
710+
{
711+
Oid *serialized = (Oid *) space;
712+
713+
/*
714+
* Make sure the hash table hasn't changed in size since the caller
715+
* reserved the space.
716+
*/
717+
Assert(size == EstimateEnumBlacklistSpace());
718+
719+
/* Write out all the values from the hash table, if there is one. */
720+
if (enum_blacklist)
721+
{
722+
HASH_SEQ_STATUS status;
723+
Oid *value;
724+
725+
hash_seq_init(&status, enum_blacklist);
726+
while ((value = (Oid *) hash_seq_search(&status)))
727+
*serialized++ = *value;
728+
}
729+
730+
/* Write out the terminator. */
731+
*serialized = InvalidOid;
732+
733+
/*
734+
* Make sure the amount of space we actually used matches what was
735+
* estimated.
736+
*/
737+
Assert((char *) (serialized + 1) == ((char *) space) + size);
738+
}
739+
740+
void
741+
RestoreEnumBlacklist(void *space)
742+
{
743+
Oid *serialized = (Oid *) space;
744+
745+
Assert(!enum_blacklist);
746+
747+
/*
748+
* As a special case, if the list is empty then don't even bother to
749+
* create the hash table. This is the usual case, since enum alteration
750+
* is expected to be rare.
751+
*/
752+
if (!OidIsValid(*serialized))
753+
return;
754+
755+
/* Read all the values into a new hash table. */
756+
init_enum_blacklist();
757+
do
758+
{
759+
hash_search(enum_blacklist, serialized++, HASH_ENTER, NULL);
760+
} while (OidIsValid(*serialized));
761+
}

‎src/backend/commands/typecmds.c

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,10 +1270,10 @@ DefineEnum(CreateEnumStmt *stmt)
12701270

12711271
/*
12721272
* AlterEnum
1273-
* ALTER TYPE on an enum.
1273+
* Adds a new label to an existing enum.
12741274
*/
12751275
ObjectAddress
1276-
AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
1276+
AlterEnum(AlterEnumStmt *stmt)
12771277
{
12781278
Oid enum_type_oid;
12791279
TypeName *typename;
@@ -1291,6 +1291,8 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
12911291
/* Check it's an enum and check user has permission to ALTER the enum */
12921292
checkEnumOwner(tup);
12931293

1294+
ReleaseSysCache(tup);
1295+
12941296
if (stmt->oldVal)
12951297
{
12961298
/* Rename an existing label */
@@ -1299,27 +1301,6 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
12991301
else
13001302
{
13011303
/* Add a new label */
1302-
1303-
/*
1304-
* Ordinarily we disallow adding values within transaction blocks,
1305-
* because we can't cope with enum OID values getting into indexes and
1306-
* then having their defining pg_enum entries go away. However, it's
1307-
* okay if the enum type was created in the current transaction, since
1308-
* then there can be no such indexes that wouldn't themselves go away
1309-
* on rollback. (We support this case because pg_dump
1310-
* --binary-upgrade needs it.) We test this by seeing if the pg_type
1311-
* row has xmin == current XID and is not HEAP_UPDATED. If it is
1312-
* HEAP_UPDATED, we can't be sure whether the type was created or only
1313-
* modified in this xact. So we are disallowing some cases that could
1314-
* theoretically be safe; but fortunately pg_dump only needs the
1315-
* simplest case.
1316-
*/
1317-
if (HeapTupleHeaderGetXmin(tup->t_data) == GetCurrentTransactionId() &&
1318-
!(tup->t_data->t_infomask & HEAP_UPDATED))
1319-
/* safe to do inside transaction block */ ;
1320-
else
1321-
PreventInTransactionBlock(isTopLevel, "ALTER TYPE ... ADD");
1322-
13231304
AddEnumLabel(enum_type_oid, stmt->newVal,
13241305
stmt->newValNeighbor, stmt->newValIsAfter,
13251306
stmt->skipIfNewValExists);
@@ -1329,8 +1310,6 @@ AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
13291310

13301311
ObjectAddressSet(address, TypeRelationId, enum_type_oid);
13311312

1332-
ReleaseSysCache(tup);
1333-
13341313
return address;
13351314
}
13361315

‎src/backend/tcop/utility.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1439,7 +1439,7 @@ ProcessUtilitySlow(ParseState *pstate,
14391439
break;
14401440

14411441
case T_AlterEnumStmt: /* ALTER TYPE (enum) */
1442-
address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
1442+
address = AlterEnum((AlterEnumStmt *) parsetree);
14431443
break;
14441444

14451445
case T_ViewStmt: /* CREATE VIEW */

0 commit comments

Comments
 (0)