Обсуждение: *_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
abug 
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

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
offerssimilar, 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
lastvalue of the enum, allowing ENUMFOO_NUM to stay correct after adding new values. Just as it was explicitly stated
beforepg18 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
someother 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
othercase 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 (: )
goalof making developers think smth like "why there is that thing” and not to break ENUMFOO_NUM accidentally when they
areadding 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.