On Wed, Jun 17, 2020 at 10:32 PM Magnus Hagander <mag...@hagander.net> wrote: > In looking at this I realize we also have exactly one thing referred to as > "blacklist" in our codebase, which is the "enum blacklist" (and then a small > internal variable in pgindent). AFAICT, it's not actually exposed to > userspace anywhere, so we could probably make the attached change to > blocklist at no "cost" (the only thing changed is the name of the hash table, > and we definitely change things like that in normal releases with no specific > thought on backwards compat).
+1 Hmm, can we find a more descriptive name for this mechanism? What about calling it the "uncommitted enum table"? See attached.
From 39dfbc83691ee9f48ea844172e38231740674aed Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 22 Oct 2020 10:09:55 +1300 Subject: [PATCH] Rename "enum blacklist" to "uncommitted enum table". Internal data structures relating to uncommitted enum values are better described that way, and the earlier term is now unpopular in the software industry for its (unintended) connotations. Discussion: https://postgr.es/m/20200615182235.x7lch5n6kcjq4aue%40alap3.anarazel.de --- src/backend/access/transam/parallel.c | 32 ++++++------- src/backend/catalog/pg_enum.c | 65 ++++++++++++++------------- src/backend/utils/adt/enum.c | 4 +- src/include/catalog/pg_enum.h | 8 ++-- 4 files changed, 56 insertions(+), 53 deletions(-) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index b0426960c7..1f499b5131 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -75,7 +75,7 @@ #define PARALLEL_KEY_PENDING_SYNCS UINT64CONST(0xFFFFFFFFFFFF000B) #define PARALLEL_KEY_REINDEX_STATE UINT64CONST(0xFFFFFFFFFFFF000C) #define PARALLEL_KEY_RELMAPPER_STATE UINT64CONST(0xFFFFFFFFFFFF000D) -#define PARALLEL_KEY_ENUMBLACKLIST UINT64CONST(0xFFFFFFFFFFFF000E) +#define PARALLEL_KEY_UNCOMMITTEDENUMTABLE UINT64CONST(0xFFFFFFFFFFFF000E) /* Fixed-size parallel state. */ typedef struct FixedParallelState @@ -211,7 +211,7 @@ InitializeParallelDSM(ParallelContext *pcxt) Size pendingsyncslen = 0; Size reindexlen = 0; Size relmapperlen = 0; - Size enumblacklistlen = 0; + Size uncommittedenumtablelen = 0; Size segsize = 0; int i; FixedParallelState *fps; @@ -267,8 +267,8 @@ 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); + uncommittedenumtablelen = EstimateUncommittedEnumTableSpace(); + shm_toc_estimate_chunk(&pcxt->estimator, uncommittedenumtablelen); /* If you add more chunks here, you probably need to add keys. */ shm_toc_estimate_keys(&pcxt->estimator, 11); @@ -348,7 +348,7 @@ InitializeParallelDSM(ParallelContext *pcxt) char *error_queue_space; char *session_dsm_handle_space; char *entrypointstate; - char *enumblacklistspace; + char *uncommittedenumtablespace; Size lnamelen; /* Serialize shared libraries we have loaded. */ @@ -404,11 +404,13 @@ 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); + /* Serialize uncommitted enum state. */ + uncommittedenumtablespace = shm_toc_allocate(pcxt->toc, + uncommittedenumtablelen); + SerializeUncommittedEnumTable(uncommittedenumtablespace, + uncommittedenumtablelen); + shm_toc_insert(pcxt->toc, PARALLEL_KEY_UNCOMMITTEDENUMTABLE, + uncommittedenumtablespace); /* Allocate space for worker information. */ pcxt->worker = palloc0(sizeof(ParallelWorkerInfo) * pcxt->nworkers); @@ -1257,7 +1259,7 @@ ParallelWorkerMain(Datum main_arg) char *pendingsyncsspace; char *reindexspace; char *relmapperspace; - char *enumblacklistspace; + char *uncommittedenumtablespace; StringInfoData msgbuf; char *session_dsm_handle_space; @@ -1449,10 +1451,10 @@ 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); + /* Restore uncommitted enum table. */ + uncommittedenumtablespace = + shm_toc_lookup(toc,PARALLEL_KEY_UNCOMMITTEDENUMTABLE, false); + RestoreUncommittedEnumTable(uncommittedenumtablespace); /* Attach to the leader's serializable transaction, if SERIALIZABLE. */ AttachSerializableXact(fps->serializable_xact_handle); diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c index 27e4100a6f..749b9dbee1 100644 --- a/src/backend/catalog/pg_enum.c +++ b/src/backend/catalog/pg_enum.c @@ -41,10 +41,11 @@ Oid binary_upgrade_next_pg_enum_oid = InvalidOid; * 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. + * 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. */ -static HTAB *enum_blacklist = NULL; +static HTAB *uncommitted_enum_table = NULL; static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems); static int sort_order_cmp(const void *p1, const void *p2); @@ -181,10 +182,10 @@ EnumValuesDelete(Oid enumTypeOid) } /* - * Initialize the enum blacklist for this transaction. + * Initialize the uncommitted enum table for this transaction. */ static void -init_enum_blacklist(void) +init_uncommitted_enum_table(void) { HASHCTL hash_ctl; @@ -192,10 +193,10 @@ init_enum_blacklist(void) 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); + uncommitted_enum_table = hash_create("Uncommitted enum table", + 32, + &hash_ctl, + HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); } /* @@ -491,12 +492,12 @@ restart: table_close(pg_enum, RowExclusiveLock); - /* Set up the blacklist hash if not already done in this transaction */ - if (enum_blacklist == NULL) - init_enum_blacklist(); + /* Set up the enum table if not already done in this transaction */ + if (uncommitted_enum_table == NULL) + init_uncommitted_enum_table(); - /* Add the new value to the blacklist */ - (void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL); + /* Add the new value to the table */ + (void) hash_search(uncommitted_enum_table, &newOid, HASH_ENTER, NULL); } @@ -585,19 +586,19 @@ RenameEnumLabel(Oid enumTypeOid, /* - * Test if the given enum value is on the blacklist + * Test if the given enum value is in the table of blocked enums. */ bool -EnumBlacklisted(Oid enum_id) +EnumUncommitted(Oid enum_id) { bool found; - /* If we've made no blacklist table, all values are safe */ - if (enum_blacklist == NULL) + /* If we've made no uncommitted table, all values are safe */ + if (uncommitted_enum_table == NULL) return false; /* Else, is it in the table? */ - (void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found); + (void) hash_search(uncommitted_enum_table, &enum_id, HASH_FIND, &found); return found; } @@ -609,11 +610,11 @@ void AtEOXact_Enum(void) { /* - * Reset the blacklist table, as all our enum values are now committed. + * 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. */ - enum_blacklist = NULL; + uncommitted_enum_table = NULL; } @@ -692,12 +693,12 @@ sort_order_cmp(const void *p1, const void *p2) } Size -EstimateEnumBlacklistSpace(void) +EstimateUncommittedEnumTableSpace(void) { size_t entries; - if (enum_blacklist) - entries = hash_get_num_entries(enum_blacklist); + if (uncommitted_enum_table) + entries = hash_get_num_entries(uncommitted_enum_table); else entries = 0; @@ -706,7 +707,7 @@ EstimateEnumBlacklistSpace(void) } void -SerializeEnumBlacklist(void *space, Size size) +SerializeUncommittedEnumTable(void *space, Size size) { Oid *serialized = (Oid *) space; @@ -714,15 +715,15 @@ SerializeEnumBlacklist(void *space, Size size) * Make sure the hash table hasn't changed in size since the caller * reserved the space. */ - Assert(size == EstimateEnumBlacklistSpace()); + Assert(size == EstimateUncommittedEnumTableSpace()); /* Write out all the values from the hash table, if there is one. */ - if (enum_blacklist) + if (uncommitted_enum_table) { HASH_SEQ_STATUS status; Oid *value; - hash_seq_init(&status, enum_blacklist); + hash_seq_init(&status, uncommitted_enum_table); while ((value = (Oid *) hash_seq_search(&status))) *serialized++ = *value; } @@ -738,11 +739,11 @@ SerializeEnumBlacklist(void *space, Size size) } void -RestoreEnumBlacklist(void *space) +RestoreUncommittedEnumTable(void *space) { Oid *serialized = (Oid *) space; - Assert(!enum_blacklist); + Assert(!uncommitted_enum_table); /* * As a special case, if the list is empty then don't even bother to @@ -753,9 +754,9 @@ RestoreEnumBlacklist(void *space) return; /* Read all the values into a new hash table. */ - init_enum_blacklist(); + init_uncommitted_enum_table(); do { - hash_search(enum_blacklist, serialized++, HASH_ENTER, NULL); + hash_search(uncommitted_enum_table, serialized++, HASH_ENTER, NULL); } while (OidIsValid(*serialized)); } diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c index 5ead794e34..ed7da7e007 100644 --- a/src/backend/utils/adt/enum.c +++ b/src/backend/utils/adt/enum.c @@ -83,12 +83,12 @@ check_safe_enum_use(HeapTuple enumval_tup) return; /* - * Check if the enum value is blacklisted. If not, it's safe, because it + * 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.) */ - if (!EnumBlacklisted(en->oid)) + if (!EnumUncommitted(en->oid)) return; /* diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h index b28d441ba7..fd90365f45 100644 --- a/src/include/catalog/pg_enum.h +++ b/src/include/catalog/pg_enum.h @@ -53,10 +53,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 bool EnumUncommitted(Oid enum_id); +extern Size EstimateUncommittedEnumTableSpace(void); +extern void SerializeUncommittedEnumTable(void *space, Size size); +extern void RestoreUncommittedEnumTable(void *space); extern void AtEOXact_Enum(void); #endif /* PG_ENUM_H */ -- 2.20.1
