Обсуждение: Add sanity check for duplicate enum values in GUC definitions

On 15.12.25 10:16, 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.

Why do you assign explicit values at all?

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

Hello

> . While working on [1]. I added an enum-typed GUC

I wanted to check the original issue, but the linked patch adds a
boolean GUC (logical_replication_fallback_to_full_identity), I did not
see enum mentioned anywhere in the diff, did you link the correct
thread?

> Ideally, such a check would run at compile time, but that would require parsing guc_tables.c with Perl

Maybe you could compile a utility executable as part of the build, and
execute it as part of the test suite?

> On Dec 17, 2025, at 22:51, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 15.12.25 10:16, 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
acopy-and-paste mistake, assigning the same numeric value to two different enum entries. This resulted in confusing
runtimebehavior and cost me about an hour to track down. 
>
> Why do you assign explicit values at all?

Hi Peter,

Did you mean to say “duplicate” instead of “explicit”?

Duplicate values assigning to different enum items was a copy-paste mistake I made during development, which wasted my
timeon debugging the issue. So I wanted to add this sanity check to quickly report such mistake in the first place. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Dec 17, 2025, at 23:19, Andres Freund <andres@anarazel.de> wrote:
>
> 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.

There is actually only one enum (“wal_compression”) having duplicate values if excluding hidden items. In my patch,
hiddenitems have been ignored, so the white-list has only one entry. 

>
> 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.
>

I think copy-paste mistake is easy to make. I have done an enhancement for GUC, see [1], that also helps debugging
againstsuch errors. In the discussion, you can see that Alvaro echoed that he made the same mistake. 

[1] https://www.postgresql.org/message-id/CAEoWx2%3DoP4LgHi771_OKhPPUS7B-CTqCs%3D%3DuQcNXWrwBoAm5Vg%40mail.gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Dec 18, 2025, at 05:32, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> Hello
>
>> . While working on [1]. I added an enum-typed GUC
>
> I wanted to check the original issue, but the linked patch adds a
> boolean GUC (logical_replication_fallback_to_full_identity), I did not
> see enum mentioned anywhere in the diff, did you link the correct
> thread?
>
>> Ideally, such a check would run at compile time, but that would require parsing guc_tables.c with Perl
>
> Maybe you could compile a utility executable as part of the build, and
> execute it as part of the test suite?

Hi Zsolt,

Thanks for asking. The link was correct. While working on the patch, I experimented with multiple solutions, one was
addinga new GUC “default_replica_identity”. 

For that, I defined a enum in guc_table.c, with items like:

```
“Default”, DEFAULT, false,
“Full”, FULL, false,
“None”, FULL, false, <== copy-paste mistake here
NULL, NULL, tue
```

I mistakenly copy FULL to the “None” line. While testing, I did “alter database xxx set default_replica_identity =
full/none”,and found that resulted the same. Mixing the fact that a GUC change doesn't take effective immediately,
sometimesneeding restart/reconnect, etc., I spent time tracking down the error, and finally identified the copy-paste
mistake.The experience triggered the idea of adding a sanity check. With this patch, such mistake will cause postmaster
failto start, so that a developer will notice the problem in the first place. That’s why I mentioned this could be a
developer-facingfeature, maybe put all code inside #ifdef USE_ASSERT_CHECKING, so that it won’t impact release version
atall. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/