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
>From b4cf37ce1d32bc251d77bfa2976324946354202c Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 26 Jan 2025 14:10:58 -0500 Subject: [PATCH v1 1/3] Move MAX_BACKENDS to pg_config_manual.h MAX_BACKENDS influences many things besides postmaster. I just noticed that we don't have static assertions ensuring BUF_REFCOUNT_MASK is big enough for MAX_BACKENDS, adding them would require including postmaster.h in buf_internals.h which doesn't seem right. Discussion: https://postgr.es/m/wptizm4qt6yikgm2pt52xzyv6ycmqiutloyvypvmagn7xvqkce@d4xuv3mylpg4 --- src/include/pg_config_manual.h | 12 ++++++++++++ src/include/postmaster/postmaster.h | 12 ------------ src/backend/storage/lmgr/lwlock.c | 1 - src/backend/utils/adt/xid8funcs.c | 1 - 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index 449e50bd78c..9874302946e 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -42,6 +42,18 @@ */ #define FUNC_MAX_ARGS 100 +/* + * Note: MAX_BACKENDS is limited to 2^18-1 because that's the width reserved + * for buffer references in buf_internals.h. This limitation could be lifted + * by using a 64bit state; but it's unlikely to be worthwhile as 2^18-1 + * backends exceed currently realistic configurations. Even if that limitation + * were removed, we still could not a) exceed 2^23-1 because inval.c stores + * the ProcNumber as a 3-byte signed integer, b) INT_MAX/4 because some places + * compute 4*MaxBackends without any overflow check. This is rechecked in the + * relevant GUC check hooks and in RegisterBackgroundWorker(). + */ +#define MAX_BACKENDS 0x3FFFF + /* * When creating a product derived from PostgreSQL with changes that cause * incompatibilities for loadable modules, it is recommended to change this diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index 188a06e2379..e12af62a370 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -126,18 +126,6 @@ extern PMChild *AllocDeadEndChild(void); extern bool ReleasePostmasterChildSlot(PMChild *pmchild); extern PMChild *FindPostmasterChildByPid(int pid); -/* - * Note: MAX_BACKENDS is limited to 2^18-1 because that's the width reserved - * for buffer references in buf_internals.h. This limitation could be lifted - * by using a 64bit state; but it's unlikely to be worthwhile as 2^18-1 - * backends exceed currently realistic configurations. Even if that limitation - * were removed, we still could not a) exceed 2^23-1 because inval.c stores - * the ProcNumber as a 3-byte signed integer, b) INT_MAX/4 because some places - * compute 4*MaxBackends without any overflow check. This is rechecked in the - * relevant GUC check hooks and in RegisterBackgroundWorker(). - */ -#define MAX_BACKENDS 0x3FFFF - /* * These values correspond to the special must-be-first options for dispatching * to various subprograms. parse_dispatch_option() can be used to convert an diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 2f558ffea14..328b1bab659 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -80,7 +80,6 @@ #include "pg_trace.h" #include "pgstat.h" #include "port/pg_bitutils.h" -#include "postmaster/postmaster.h" #include "storage/proc.h" #include "storage/proclist.h" #include "storage/spin.h" diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c index 20b28b2528c..d505e3fcf57 100644 --- a/src/backend/utils/adt/xid8funcs.c +++ b/src/backend/utils/adt/xid8funcs.c @@ -32,7 +32,6 @@ #include "lib/qunique.h" #include "libpq/pqformat.h" #include "miscadmin.h" -#include "postmaster/postmaster.h" #include "storage/lwlock.h" #include "storage/procarray.h" #include "utils/builtins.h" -- 2.48.1.76.g4e746b1a31.dirty
>From 02fbbbf691ab28f267f5426bb4f3ba7c8cd950dc Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 26 Jan 2025 14:13:57 -0500 Subject: [PATCH v1 2/3] bufmgr: Assert that MAX_BACKENDS is compatible with BufferDesc.state Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/include/storage/buf_internals.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 1a65342177d..d93493f746b 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -39,8 +39,9 @@ * * The definition of buffer state components is below. */ +#define BUF_REFCOUNT_BITS 18 #define BUF_REFCOUNT_ONE 1 -#define BUF_REFCOUNT_MASK ((1U << 18) - 1) +#define BUF_REFCOUNT_MASK ((1U << BUF_REFCOUNT_BITS) - 1) #define BUF_USAGECOUNT_MASK 0x003C0000U #define BUF_USAGECOUNT_ONE (1U << 18) #define BUF_USAGECOUNT_SHIFT 18 @@ -77,6 +78,9 @@ */ #define BM_MAX_USAGE_COUNT 5 +StaticAssertDecl(MAX_BACKENDS <= ((1 << BUF_REFCOUNT_BITS) - 1), + "MAX_BACKENDS is too big for BUF_REFCOUNT_BITS"); + /* * Buffer tag identifies which disk block the buffer contains. * -- 2.48.1.76.g4e746b1a31.dirty
>From 80e4d720dddef6d354e8d6fa523b08c29f9fcc75 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 26 Jan 2025 14:15:31 -0500 Subject: [PATCH v1 3/3] WIP: Base LWLock limits directly on MAX_BACKENDS Jacob reported that comments for LW_SHARED_MASK referenced a MAX_BACKENDS limit of 2^23-1, but that MAX_BACKENDS is actually limited to 2^18-1. The limit was lowered in 48354581a49c, but the comment in lwlock.c wasn't updated. Instead of just fixing the comment, it seems better to directly base the lwlock defines on MAX_BACKENDS and add static assertions to ensure that there is enough space. That way there's no comment that can go out of sync in the future. As part of that change I noticed that for some reason the high bit wasn't used for flags, which seems somewhat odd. Redefine the flag values to start at the highest bit. Reported-by: Jacob Brazeal <jacob.braz...@gmail.com> Discussion: https://postgr.es/m/CA+COZaBO_s3LfALq=b+hcbhfsoegiapvjrracce4vp9m7cj...@mail.gmail.com --- src/include/pg_config_manual.h | 4 +++- src/backend/storage/lmgr/lwlock.c | 26 ++++++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index 9874302946e..55db22eb095 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -51,8 +51,10 @@ * the ProcNumber as a 3-byte signed integer, b) INT_MAX/4 because some places * compute 4*MaxBackends without any overflow check. This is rechecked in the * relevant GUC check hooks and in RegisterBackgroundWorker(). + * + * Various places currently assume that MAX_BACKENDS + 1 is a power of two. */ -#define MAX_BACKENDS 0x3FFFF +#define MAX_BACKENDS 0x3FFFFU /* * When creating a product derived from PostgreSQL with changes that cause diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 328b1bab659..eea51ec601c 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -90,18 +90,28 @@ #endif -#define LW_FLAG_HAS_WAITERS ((uint32) 1 << 30) -#define LW_FLAG_RELEASE_OK ((uint32) 1 << 29) -#define LW_FLAG_LOCKED ((uint32) 1 << 28) +#define LW_FLAG_HAS_WAITERS ((uint32) 1 << 31) +#define LW_FLAG_RELEASE_OK ((uint32) 1 << 30) +#define LW_FLAG_LOCKED ((uint32) 1 << 29) +#define LW_FLAG_BITS 3 +#define LW_FLAG_MASK (((1<<LW_FLAG_BITS)-1)<<(32-LW_FLAG_BITS)) -#define LW_VAL_EXCLUSIVE ((uint32) 1 << 24) +/* assumed MAX_BACKENDS is a (power of 2) - 1, checked below */ +#define LW_VAL_EXCLUSIVE (MAX_BACKENDS + 1) #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. */ -#define LW_SHARED_MASK ((uint32) ((1 << 24)-1)) +/* already (power of 2)-1, i.e. suitable for a mask */ +#define LW_SHARED_MASK MAX_BACKENDS +#define LW_LOCK_MASK (MAX_BACKENDS | LW_VAL_EXCLUSIVE) -StaticAssertDecl(LW_VAL_EXCLUSIVE > (uint32) MAX_BACKENDS, + +StaticAssertDecl(((MAX_BACKENDS + 1) & MAX_BACKENDS) == 0, + "MAX_BACKENDS + 1 needs to be a power of 2"); + +StaticAssertDecl((MAX_BACKENDS & LW_FLAG_MASK) == 0, + "MAX_BACKENDS and LW_FLAG_MASK overlap"); + +StaticAssertDecl(MAX_BACKENDS < LW_VAL_EXCLUSIVE, "MAX_BACKENDS too big for lwlock.c"); /* -- 2.48.1.76.g4e746b1a31.dirty
