*_LAST in enums to define NUM* macross
Hi all!
Recently we rebased one of our internal patches, which defines new ProcSignalReason value onto pgsql18 and encountered a bug
related to NUM_PROCSIGNALS. The patch was written for and older pgsql version, where NUM_PROCSIGNALS was defined
as value of the enum, but now it is equal to (PROCSIG_RECOVERY_CONFLICT_LAST + 1).
As the result, the rebase (which was successful) led to incorrect value of NUM_PROCSIGNALS and runtime error
with usage of pss_signalFlags.
I thought about how to prevent such errors in the future, and realized that the recipe for this is already in
ProcSignalReason - you need to declare LAST elements equal to one of the enum values and use them when declaring NUM macros.
Based on commit 10b7218, I extended this pattern by adding *_LAST elements and updating NUM-like macros in other
modified enums within this patch (see attachment).
Please note, that this will not lead to extra compiler warnings about writing switch statements on enum values,
because LAST value in enums is always equal to some other value of the enum.
Any thoughts or suggestions on this approach?
—
Best regards,
Roman Khapov
Attachments:
0001-use-_LAST-in-enums-to-define-NUM-macross.patchapplication/octet-stream; name=0001-use-_LAST-in-enums-to-define-NUM-macross.patch; x-unix-mode=0644Download
From a8621383ed770963d4a6c461753f8c4d0533a193 Mon Sep 17 00:00:00 2001
From: roman khapov <r.khapov@ya.ru>
Date: Tue, 18 Nov 2025 18:15:24 +0500
Subject: [PATCH] use _LAST in enums to define NUM macross
Inspired by 10b7218.
Add *_LAST enums that mark the last valid value,
to be used in *_NUM macros calculating enum size.
This helps avoid mistakes in NUM maceoss when adding
new values in enums.
Note: this will not lead to additional compiler warnings
because _LAST is always equals to some other value of enum.
Signed-off-by: roman khapov <r.khapov@ya.ru>
---
contrib/pg_stat_statements/pg_stat_statements.c | 4 +++-
src/backend/postmaster/autovacuum.c | 4 +++-
src/bin/pg_dump/pg_backup.h | 4 +++-
src/include/miscadmin.h | 4 +++-
src/include/storage/pmsignal.h | 4 +++-
src/include/storage/procsignal.h | 1 +
6 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 39208f80b5b..2cc6e51b964 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -129,9 +129,11 @@ typedef enum pgssStoreKind
*/
PGSS_PLAN = 0,
PGSS_EXEC,
+
+ PGSS_LAST = PGSS_EXEC,
} pgssStoreKind;
-#define PGSS_NUMKIND (PGSS_EXEC + 1)
+#define PGSS_NUMKIND (PGSS_LAST + 1)
/*
* Hashtable key that defines the identity of a hashtable entry. We separate
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 1c38488f2cb..47f7ea68367 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -250,9 +250,11 @@ typedef enum
{
AutoVacForkFailed, /* failed trying to start a worker */
AutoVacRebalance, /* rebalance the cost limits */
+
+ AutoVacLast = AutoVacRebalance,
} AutoVacuumSignal;
-#define AutoVacNumSignals (AutoVacRebalance + 1)
+#define AutoVacNumSignals (AutoVacLast + 1)
/*
* Autovacuum workitem array, stored in AutoVacuumShmem->av_workItems. This
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index d9041dad720..299960b8fb8 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -75,9 +75,11 @@ enum _dumpPreparedQueries
PREPQUERY_GETATTRIBUTESTATS,
PREPQUERY_GETCOLUMNACLS,
PREPQUERY_GETDOMAINCONSTRAINTS,
+
+ PREPQUERY_LAST = PREPQUERY_GETDOMAINCONSTRAINTS,
};
-#define NUM_PREP_QUERIES (PREPQUERY_GETDOMAINCONSTRAINTS + 1)
+#define NUM_PREP_QUERIES (PREPQUERY_LAST + 1)
/* Parameters needed by ConnectDatabase; same for dump and restore */
typedef struct _connParams
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 9a7d733ddef..cbcfda4e791 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -372,9 +372,11 @@ typedef enum BackendType
* entry.
*/
B_LOGGER,
+
+ B_LAST = B_LOGGER,
} BackendType;
-#define BACKEND_NUM_TYPES (B_LOGGER + 1)
+#define BACKEND_NUM_TYPES (B_LAST + 1)
extern PGDLLIMPORT BackendType MyBackendType;
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 428aa3fd68a..0b0fca85fdf 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -42,9 +42,11 @@ typedef enum
PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
PMSIGNAL_XLOG_IS_SHUTDOWN, /* ShutdownXLOG() completed */
+
+ PGSIGNAL_XLOG_LAST = PMSIGNAL_XLOG_IS_SHUTDOWN,
} PMSignalReason;
-#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)
+#define NUM_PMSIGNALS (PGSIGNAL_XLOG_LAST+1)
/*
* Reasons why the postmaster would send SIGQUIT to its children.
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index afeeb1ca019..d0f925b986b 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -46,6 +46,7 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_LOGICALSLOT,
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
+
PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
} ProcSignalReason;
--
2.50.1 (Apple Git-155)
Roman Khapov <rkhapov@yandex-team.ru> writes:
Based on commit 10b7218, I extended this pattern by adding *_LAST elements and updating NUM-like macros in other
modified enums within this patch (see attachment).
Isn't this patch basically proposing to revert 10b7218?
How has the situation changed since then to make that
a good idea?
Please note, that this will not lead to extra compiler warnings about writing switch statements on enum values,
because LAST value in enums is always equal to some other value of the enum.
TBH I find that argument wildly optimistic. Even if it's true
today it could stop being true next week.
I don't really like the FOOENUM_LAST style at all. It's ugly, it
makes a dubious assumption about compiler behavior, and it doesn't
look even one bit safer than the NUM_FOOENUM style. Either way,
when adding a new enum value there is another place that you have
to remember to update.
Admittedly, with FOOENUM_LAST the "other place" is one line closer
than with a separate NUM_FOOENUM macro, and we have seen repeatedly
that people don't read code more than two lines away from what they
are patching :-(. But we could narrow that gap without making any
new assumptions, like this:
PMSIGNAL_BACKGROUND_WORKER_CHANGE, /* background worker state change */
PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
PMSIGNAL_XLOG_IS_SHUTDOWN, /* ShutdownXLOG() completed */
+
+#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)
} PMSignalReason;
-
-#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)(I'm not actually advocating such a patch, because I doubt it's
worth the trouble. But perhaps others will think it worthwhile.)
regards, tom lane
Thanks for your feedback!
On 18 Nov 2025, at 21:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Isn't this patch basically proposing to revert 10b7218?
How has the situation changed since then to make that
a good idea?
No, it is not a revert, until we don’t have a separated ENUMFOO_NUM value with numeric value as <last> + 1, this patch offers similar, but still slightly different way to define that value..
I was hoping that when adding new values, the developer would think "why there is a ENUMFOO_LAST here" and leave it the last value of the enum, allowing ENUMFOO_NUM to stay correct after adding new values. Just as it was explicitly stated before pg18 in comment in enum definition, but at the same time I wanted to get rid of the compiler warnings.
Please note, that this will not lead to extra compiler warnings about writing switch statements on enum values,
because LAST value in enums is always equal to some other value of the enum.TBH I find that argument wildly optimistic. Even if it's true
today it could stop being true next week.
Hmm, I don’t see any reason, why compilers can add warnings about missing enum value, that int representation equals to some other enum's value. It would be an correct warning if you forget to write case for value, that is equals to ENUMFOO_LAST, or it would be a bug in compiler, if it offers you to write case statements that is already covered some other case statement.
PMSIGNAL_BACKGROUND_WORKER_CHANGE, /* background worker state change */ PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */ PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */ PMSIGNAL_XLOG_IS_SHUTDOWN, /* ShutdownXLOG() completed */ + +#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1) } PMSignalReason; - -#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)
Anyway, I like your variant of defining ENUMFOO_NUM inside enum, because it allows to achieve (or at least try to (: ) goal of making developers think smth like "why there is that thing” and not to break ENUMFOO_NUM accidentally when they are adding new ENUMFOO_VALUE in the end of the enum.
(I'm not actually advocating such a patch, because I doubt it's
worth the trouble. But perhaps others will think it worthwhile.)
Still, I made a patch v2, in case someone decides that it would be useful.
Attachments:
v2-0001-move-ENUMFOO_NUM-definitions-inside-enums.patchapplication/octet-stream; name=v2-0001-move-ENUMFOO_NUM-definitions-inside-enums.patch; x-unix-mode=0644Download
From 78c7d5e46bd64f31e3b983886bc09d27d1acd334 Mon Sep 17 00:00:00 2001
From: roman khapov <r.khapov@ya.ru>
Date: Tue, 18 Nov 2025 23:29:23 +0500
Subject: [PATCH v2] move ENUMFOO_NUM definitions inside enums
Inspired by 10b7218.
To make connectivity of ENUMFOO_NUM definition
and the enum more obvious.
This helps avoid mistakes in NUM macros when adding
new values in enums.
Signed-off-by: roman khapov <r.khapov@ya.ru>
---
contrib/pg_stat_statements/pg_stat_statements.c | 2 +-
src/backend/postmaster/autovacuum.c | 2 +-
src/bin/pg_dump/pg_backup.h | 2 +-
src/include/miscadmin.h | 2 +-
src/include/nodes/primnodes.h | 2 +-
src/include/storage/pmsignal.h | 4 ++--
src/include/storage/procsignal.h | 2 +-
7 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 39208f80b5b..d4823602452 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -129,9 +129,9 @@ typedef enum pgssStoreKind
*/
PGSS_PLAN = 0,
PGSS_EXEC,
-} pgssStoreKind;
#define PGSS_NUMKIND (PGSS_EXEC + 1)
+} pgssStoreKind;
/*
* Hashtable key that defines the identity of a hashtable entry. We separate
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 1c38488f2cb..2e5c5de3339 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -250,9 +250,9 @@ typedef enum
{
AutoVacForkFailed, /* failed trying to start a worker */
AutoVacRebalance, /* rebalance the cost limits */
-} AutoVacuumSignal;
#define AutoVacNumSignals (AutoVacRebalance + 1)
+} AutoVacuumSignal;
/*
* Autovacuum workitem array, stored in AutoVacuumShmem->av_workItems. This
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index d9041dad720..213078ba054 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -75,9 +75,9 @@ enum _dumpPreparedQueries
PREPQUERY_GETATTRIBUTESTATS,
PREPQUERY_GETCOLUMNACLS,
PREPQUERY_GETDOMAINCONSTRAINTS,
-};
#define NUM_PREP_QUERIES (PREPQUERY_GETDOMAINCONSTRAINTS + 1)
+};
/* Parameters needed by ConnectDatabase; same for dump and restore */
typedef struct _connParams
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 9a7d733ddef..bf9b3f02ab2 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -372,9 +372,9 @@ typedef enum BackendType
* entry.
*/
B_LOGGER,
-} BackendType;
#define BACKEND_NUM_TYPES (B_LOGGER + 1)
+} BackendType;
extern PGDLLIMPORT BackendType MyBackendType;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 1b4436f2ff6..16f922db2b4 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -2021,9 +2021,9 @@ typedef enum MergeMatchKind
MERGE_WHEN_MATCHED,
MERGE_WHEN_NOT_MATCHED_BY_SOURCE,
MERGE_WHEN_NOT_MATCHED_BY_TARGET
-} MergeMatchKind;
#define NUM_MERGE_MATCH_KINDS (MERGE_WHEN_NOT_MATCHED_BY_TARGET + 1)
+} MergeMatchKind;
typedef struct MergeAction
{
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 428aa3fd68a..9dc7e88476a 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -42,9 +42,9 @@ typedef enum
PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
PMSIGNAL_XLOG_IS_SHUTDOWN, /* ShutdownXLOG() completed */
-} PMSignalReason;
-#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN+1)
+#define NUM_PMSIGNALS (PMSIGNAL_XLOG_IS_SHUTDOWN + 1)
+} PMSignalReason;
/*
* Reasons why the postmaster would send SIGQUIT to its children.
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index afeeb1ca019..878c4baa024 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -47,9 +47,9 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
-} ProcSignalReason;
#define NUM_PROCSIGNALS (PROCSIG_RECOVERY_CONFLICT_LAST + 1)
+} ProcSignalReason;
typedef enum
{
--
2.50.1 (Apple Git-155)