Thread

Re: Add sanity check for duplicate enum values in GUC definitions

Andres Freund <andres@anarazel.de> — 2025-12-17T15:19:54Z

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