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]
/messages/by-id/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 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 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 time on 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, hidden items 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]/messages/by-id/CAEoWx2=oP4LgHi771_OKhPPUS7B-CTqCs==uQcNXWrwBoAm5Vg@mail.gmail.com, that also helps debugging against such errors. In the discussion, you can see that Alvaro echoed that he made the same mistake.

[1]: /messages/by-id/CAEoWx2=oP4LgHi771_OKhPPUS7B-CTqCs==uQcNXWrwBoAm5Vg@mail.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 adding a 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, sometimes needing 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 fail to start, so that a developer will notice the problem in the first place. That’s why I mentioned this could be a developer-facing feature, maybe put all code inside #ifdef USE_ASSERT_CHECKING, so that it won’t impact release version at all.

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

On 18.12.25 01:22, Chao Li wrote:

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

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

No, I meant explicit. I didn't find an example in the thread you linked
to, but I suppose you are writing something like

enum foo {
bar = 1,
baz = 2,
};

But why make those assignments at all. You could just write

enum foo {
bar,
baz,
};

On Dec 18, 2025, at 15:43, Peter Eisentraut <peter@eisentraut.org> wrote:

On 18.12.25 01:22, Chao Li wrote:

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

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

No, I meant explicit. I didn't find an example in the thread you linked to, but I suppose you are writing something like

enum foo {
bar = 1,
baz = 2,
};

But why make those assignments at all. You could just write

enum foo {
bar,
baz,
};

Oh, I got your question. That's not C enum, it’s about the GUC config_enum_entry. In the reply to Zsolt, I explained what I experienced.

Thanks for asking. The link was correct. While working on the patch, I experimented with multiple solutions, one was adding a 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, sometimes needing 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 fail to start, so that a developer will notice the problem in the first place. That’s why I mentioned this could be a developer-facing feature, maybe put all code inside #ifdef USE_ASSERT_CHECKING, so that it won’t impact release version at all.

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

On Dec 18, 2025, at 15:52, Chao Li <li.evan.chao@gmail.com> wrote:

On Dec 18, 2025, at 15:43, Peter Eisentraut <peter@eisentraut.org> wrote:

On 18.12.25 01:22, Chao Li wrote:

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

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

No, I meant explicit. I didn't find an example in the thread you linked to, but I suppose you are writing something like

enum foo {
bar = 1,
baz = 2,
};

But why make those assignments at all. You could just write

enum foo {
bar,
baz,
};

Oh, I got your question. That's not C enum, it’s about the GUC config_enum_entry. In the reply to Zsolt, I explained what I experienced.

Thanks for asking. The link was correct. While working on the patch, I experimented with multiple solutions, one was adding a 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, sometimes needing 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 fail to start, so that a developer will notice the problem in the first place. That’s why I mentioned this could be a developer-facing feature, maybe put all code inside #ifdef USE_ASSERT_CHECKING, so that it won’t impact release version at all.

By the way, CF entry: https://commitfest.postgresql.org/patch/6316/

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