Hi, On 2025-12-15 17:16:56 +0800, Chao Li wrote: > The motivation for this patch comes from my own experience. While working > on [1]. I added an enum-typed GUC and made a copy-and-paste mistake, > assigning the same numeric value to two different enum entries. This > resulted in confusing runtime behavior and cost me about an hour to track > down.
I think this is something we explicitly do *not* want. It can make plenty sense to have enums with the same value assigned multiple times. And ending up with a list of exceptions doesn't strike me as a good use of time. I just dont' believe this is a frequent enough issue to be worth in-core infrastructure, particularly in light of it sometimes being actually intentionally used. Greetings, Anres > I think adding a sanity check for enum-typed GUCs would help catch this > class of mistake early. Ideally, such a check would run at compile time, > but that would require parsing guc_tables.c with Perl. I’m not very > familiar with Perl, so in this patch I instead added the check when > building the GUC hash table. > > There are a few existing enum GUCs that intentionally reuse numeric values > (for example, to provide aliases), so I introduced a small whitelist for > those cases. If a new GUC needs duplicate enum values in the future, its > name can simply be added to that list. > > This check is intended as a developer-facing sanity guard to catch mistakes > in GUC enum tables. Given that intent, once a duplicate value is detected > the backend aborts early. From that perspective, it might also make sense > to place this check under #ifdef USE_ASSERT_CHECKING. I’m interested in > hearing thoughts on that approach. > > [1] > https://postgr.es/m/caeowx2mmorbmwjkbt4ycsjdyl3r9mp+z0bbk57vz+okjtgj...@mail.gmail.com > > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > From 3c2c25d19da3b39f57c7d04b17077e015aa82758 Mon Sep 17 00:00:00 2001 > From: "Chao Li (Evan)" <[email protected]> > Date: Mon, 15 Dec 2025 17:00:59 +0800 > Subject: [PATCH v1] Add sanity check for duplicate enum values in GUC > definitions > > Add a backend startup check that scans enum-type GUCs and reports cases > where multiple enum labels map to the same numeric value. > > Historically, duplicate enum values have occasionally been introduced > unintentionally, which can make enum lookup ambiguous and harder to > reason about. This change detects such cases early, after the GUC hash > table is built, and errors out to prevent silently accepting ambiguous > definitions. > > A small number of existing enum GUCs intentionally reuse values (for > example, to provide backward-compatible aliases). These are explicitly > excluded from the check via a whitelist. > > This check is intended as a developer-facing sanity guard to catch > mistakes in GUC enum tables. > > Author: Chao Li <[email protected]> > --- > src/backend/utils/misc/guc.c | 63 +++++++++++++++++++++++++++++ > src/backend/utils/misc/guc_tables.c | 10 +++++ > src/include/utils/guc_tables.h | 2 + > 3 files changed, 75 insertions(+) > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > index 935c235e1b3..58d1eada183 100644 > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > @@ -238,6 +238,7 @@ static int guc_name_match(const void *key1, const > void *key2, Size keysize); > static void InitializeGUCOptionsFromEnvironment(void); > static void InitializeOneGUCOption(struct config_generic *gconf); > static void RemoveGUCFromLists(struct config_generic *gconf); > +static void check_enum_duplicates(void); > static void set_guc_source(struct config_generic *gconf, GucSource > newsource); > static void pg_timezone_abbrev_initialize(void); > static void push_old_value(struct config_generic *gconf, GucAction action); > @@ -860,6 +861,63 @@ get_guc_variables(int *num_vars) > return result; > } > > +/* > + * If an enum-typed GUC has duplicate values for different names, raise an > error. > + * There are a few exceptions, listed in enum_dup_whitelist. > + */ > +static void > +check_enum_duplicates(void) > +{ > + HASH_SEQ_STATUS status; > + GUCHashEntry *hentry; > + > + Assert(guc_hashtab != NULL); > + > + hash_seq_init(&status, guc_hashtab); > + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) > + { > + const struct config_generic *gconf = hentry->gucvar; > + > + if (gconf->vartype == PGC_ENUM) > + { > + bool allow_dups = false; > + > + for (const char *const *p = enum_dup_whitelist; *p; p++) > + { > + if (strcmp(gconf->name, *p) == 0) > + { > + allow_dups = true; > + break; > + } > + } > + if (allow_dups) > + continue; > + > + for (const struct config_enum_entry *opt1 = > gconf->_enum.options; opt1->name; opt1++) > + { > + if (opt1->hidden) > + continue; > + > + for (const struct config_enum_entry *opt2 = > opt1 + 1; opt2->name; opt2++) > + { > + if (opt2->hidden) > + continue; > + > + if (opt1->val == opt2->val) > + { > + elog(ERROR, > + "GUC enum \"%s\" has > duplicate value %d for \"%s\" and \"%s\"", > + gconf->name, > + opt1->val, > + opt1->name, > + opt2->name); > + } > + } > + } > + } > + } > +} > + > > /* > * Build the GUC hash table. This is split out so that help_config.c can > @@ -1420,6 +1478,11 @@ InitializeGUCOptions(void) > */ > build_guc_variables(); > > + /* > + * Check for enum GUCs with duplicate values. > + */ > + check_enum_duplicates(); > + > /* > * Load all variables with their compiled-in defaults, and initialize > * status fields as needed. > diff --git a/src/backend/utils/misc/guc_tables.c > b/src/backend/utils/misc/guc_tables.c > index f87b558c2c6..0eb8dfdf9ff 100644 > --- a/src/backend/utils/misc/guc_tables.c > +++ b/src/backend/utils/misc/guc_tables.c > @@ -764,6 +764,16 @@ const char *const config_type_names[] = > [PGC_ENUM] = "enum", > }; > > +/* > + * Enum GUCs where duplicate numeric values are expected, so they will be > + * skipped in enum sanity checks. Hidden enum values are not checked, as they > + * are not exposed to users. > + */ > +const char *enum_dup_whitelist[] = { > + "wal_compression", > + NULL > +}; > + > StaticAssertDecl(lengthof(config_type_names) == (PGC_ENUM + 1), > "array length mismatch"); > > diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h > index 04cc60eb526..04f5330fee4 100644 > --- a/src/include/utils/guc_tables.h > +++ b/src/include/utils/guc_tables.h > @@ -327,6 +327,8 @@ extern struct config_generic **get_guc_variables(int > *num_vars); > > extern void build_guc_variables(void); > > +extern const char *enum_dup_whitelist[]; > + > /* search in enum options */ > extern const char *config_enum_lookup_by_value(const struct config_generic > *record, int val); > extern bool config_enum_lookup_by_name(const struct config_enum *record, > -- > 2.39.5 (Apple Git-154) > Greetings, Andres Freund
