Re: Add sanity check for duplicate enum values in GUC definitions - Mailing list pgsql-hackers

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+OkJTgJQVQ@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)" <lic@highgo.com>
> 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 <lic@highgo.com>
> ---
>  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