Re: MAX_BACKENDS size (comment accuracy)

Hi,

On 2025-01-26 13:10:35 -0500, Andres Freund wrote:
> On 2025-01-25 16:06:29 -0800, Jacob Brazeal wrote:
> > diff --git a/src/backend/storage/lmgr/lwlock.c
> > b/src/backend/storage/lmgr/lwlock.c index 2f558ffea1..d3a2619072 100644 ---
> > a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c
> > @@ -99,7 +99,7 @@ #define LW_VAL_SHARED 1
> >
> >  #define LW_LOCK_MASK				((uint32) ((1 << 25)-1))
> > -/* Must be greater than MAX_BACKENDS - which is 2^23-1, so we're fine. */
> > +/* Must be greater than MAX_BACKENDS - which is 2^18-1, so we're fine. */
> >  #define LW_SHARED_MASK				((uint32) ((1 << 24)-1))
> >
> >  StaticAssertDecl(LW_VAL_EXCLUSIVE > (uint32) MAX_BACKENDS,
> 
> I'm inclined to think that we should adjust the various constants at the same
> time as the comment? It's imo somewhat weird to have bits LW_SHARED_MASK that
> can never be set...
> 
> I wonder if we should instead define LW_VAL_EXCLUSIVE and LW_SHARED_MASK using
> MAX_BACKENDS and have a static assertion ensuring it doesn't overlap with flag
> bits?  That way we don't have two sets of defines to keep in sync.

Started to write a patch to do what I described. Increased MAX_BACKENDS to
check if the assertions work. They did - but I noticed that we do *not* have
such assertions to check the buf_internals.h assumptions aren't violated.

Adding them would currently require including postmaster.h into
buf_internals.h, which seems somewhat gross.  I wonder if we should move the
define to pg_config_manual.h?  Seems at least somewhat more appropriate than
postmaster.h?  Only allows to remove two postmaster.h includes though, due to
all things like ClientAuthInProgress (listed as a GUC, but isn't) and GUCs
declared in postmaster.h.  So maybe it's not worth it.

Attached is a draft patch series.

Thoughts?

Andres Freund