Hi, On 2025-01-26 12:55:15 -0800, Jacob Brazeal wrote: > The patch series looks good.
Thanks for reviewing! And sorry for not getting back to you about your review comments earlier. > > StaticAssertDecl((MAX_BACKENDS & LW_FLAG_MASK) == 0, > > "MAX_BACKENDS and LW_FLAG_MASK overlap"); > > Should this check that MAX_BACKENDS & LW_LOCK_MASK == 0? To also ensure the > LW_VAL_EXCLUSIVE bit does not overlap. Yes, I think you're right. On 2025-01-26 12:57:50 -0800, Jacob Brazeal wrote: > Looking at v1-0003-WIP-Base-LWLock-limits-directly-on-MAX_BACKENDS.patch, > I'm curious about the following assert; > > > #define LW_VAL_EXCLUSIVE (MAX_BACKENDS + 1) > ... > > StaticAssertDecl(MAX_BACKENDS < LW_VAL_EXCLUSIVE, > "MAX_BACKENDS too big for lwlock.c"); > > Since LW_VAL_EXCLUSIVE is already defined as MAX_BACKENDS + 1, is this > basically just checking for an integer overflow? You're right, it's redundant. I think I added that in an earlier version and then didn't remove it. Updated patches attached. I didn't really like moving MAX_BACKENDS to pg_config_manual.h, there are too many constraints on it. I moved it to procnumber.h. That's maybe not perfect, but seems a bit better? I also introduced MAX_BACKENDS_BITS, so that it's easier to check against when asserting restrictions on bit space. I also added a patch from a different thread, to remove a bunch of the magic constants in buf_internals.h, as it felt awkward to assert the refcount bits alone were compatible with MAX_BACKENDS, when a 'standalone' 18 was used in a bunch of other places. As Heikki had already reviewed that quite a while ago, I'll push that soon. I chose to not directly base BUF_REFCOUNT_BITS directly on MAX_BACKEND_BITS, as it's ok to lower MAX_BACKEND_BITS without adjusting BUF_REFCOUNT_BITS. But I went back and forth on that one. Greetings, Andres Freund
>From 2fd801aad661d8c8de816f10c014edd7705dd3f0 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 26 Jan 2025 14:10:58 -0500 Subject: [PATCH v2 1/4] Move MAX_BACKENDS to procnumber.h MAX_BACKENDS influences many things besides postmaster. I e.g. 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. While at that, add MAX_BACKENDS_BITS, as that's useful in various places for static assertions (to be added in subsequent commits). Discussion: https://postgr.es/m/wptizm4qt6yikgm2pt52xzyv6ycmqiutloyvypvmagn7xvqkce@d4xuv3mylpg4 --- src/include/postmaster/postmaster.h | 12 ------------ src/include/storage/procnumber.h | 13 +++++++++++++ src/backend/storage/lmgr/lwlock.c | 2 +- src/backend/utils/adt/xid8funcs.c | 2 +- src/backend/utils/init/postinit.c | 1 + src/backend/utils/misc/guc_tables.c | 1 + 6 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index d8a9f14b3b8..b6a3f275a1b 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/include/storage/procnumber.h b/src/include/storage/procnumber.h index 7cf981ab673..fdbecc55dbe 100644 --- a/src/include/storage/procnumber.h +++ b/src/include/storage/procnumber.h @@ -25,6 +25,19 @@ typedef int ProcNumber; #define INVALID_PROC_NUMBER (-1) +/* + * Note: MAX_BACKENDS_BITS is 18 as that is the space available for buffer + * refcounts 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_BITS 18 +#define MAX_BACKENDS ((1 << MAX_BACKENDS_BITS)-1) + /* * Proc number of this backend (same as GetNumberFromPGProc(MyProc)) */ diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index f1e74f184f1..d08144e9c22 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -80,9 +80,9 @@ #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/procnumber.h" #include "storage/spin.h" #include "utils/memutils.h" diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c index 20b28b2528c..88d798fbf4b 100644 --- a/src/backend/utils/adt/xid8funcs.c +++ b/src/backend/utils/adt/xid8funcs.c @@ -32,9 +32,9 @@ #include "lib/qunique.h" #include "libpq/pqformat.h" #include "miscadmin.h" -#include "postmaster/postmaster.h" #include "storage/lwlock.h" #include "storage/procarray.h" +#include "storage/procnumber.h" #include "utils/builtins.h" #include "utils/memutils.h" #include "utils/snapmgr.h" diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 01bb6a410cb..318600d6d02 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -49,6 +49,7 @@ #include "storage/lmgr.h" #include "storage/proc.h" #include "storage/procarray.h" +#include "storage/procnumber.h" #include "storage/procsignal.h" #include "storage/sinvaladt.h" #include "storage/smgr.h" diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 03a6dd49154..ad25cbb39c5 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -77,6 +77,7 @@ #include "storage/large_object.h" #include "storage/pg_shmem.h" #include "storage/predicate.h" +#include "storage/procnumber.h" #include "storage/standby.h" #include "tcop/backend_startup.h" #include "tcop/tcopprot.h" -- 2.48.1.76.g4e746b1a31.dirty
>From 78253592e0ee2e5f36d62ea8789271bb7a28fea7 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Fri, 21 Feb 2025 12:59:56 -0500 Subject: [PATCH v2 2/4] 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> Reviewed-by: Jacob Brazeal <jacob.braz...@gmail.com> Discussion: https://postgr.es/m/CA+COZaBO_s3LfALq=b+hcbhfsoegiapvjrracce4vp9m7cj...@mail.gmail.com --- src/backend/storage/lmgr/lwlock.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index d08144e9c22..7c7bf3f300c 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -91,19 +91,29 @@ #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) +/* assumes 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, - "MAX_BACKENDS too big for lwlock.c"); + +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((LW_VAL_EXCLUSIVE & LW_FLAG_MASK) == 0, + "LW_VAL_EXCLUSIVE and LW_FLAG_MASK overlap"); /* * There are three sorts of LWLock "tranches": -- 2.48.1.76.g4e746b1a31.dirty
>From 716e11a592fd7df6a7e263d9b6237ff96d98c4db Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Fri, 21 Feb 2025 13:08:11 -0500 Subject: [PATCH v2 3/4] bufmgr: Make it easier to change number of buffer state bits In an upcoming commit I'd like to change the number of bits for the usage count (the current max is 5, fitting in three bits, but we reserve four bits). Until now that required adjusting a bunch of magic constants, now the constants are defined based on the number of bits reserved. Reviewed-by: Heikki Linnakangas <hlinn...@iki.fi> Discussion: https://postgr.es/m/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6@lzx3sjsqhmt6 --- src/include/storage/buf_internals.h | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 1a65342177d..d830d5c9841 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -39,12 +39,19 @@ * * The definition of buffer state components is below. */ +#define BUF_REFCOUNT_BITS 18 +#define BUF_USAGECOUNT_BITS 4 +#define BUF_FLAG_BITS 10 + +StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32, + "parts of buffer state space need to equal 32"); + #define BUF_REFCOUNT_ONE 1 -#define BUF_REFCOUNT_MASK ((1U << 18) - 1) -#define BUF_USAGECOUNT_MASK 0x003C0000U -#define BUF_USAGECOUNT_ONE (1U << 18) -#define BUF_USAGECOUNT_SHIFT 18 -#define BUF_FLAG_MASK 0xFFC00000U +#define BUF_REFCOUNT_MASK ((1U << BUF_REFCOUNT_BITS) - 1) +#define BUF_USAGECOUNT_MASK (((1U << BUF_USAGECOUNT_BITS) - 1) << (BUF_REFCOUNT_BITS)) +#define BUF_USAGECOUNT_ONE (1U << BUF_REFCOUNT_BITS) +#define BUF_USAGECOUNT_SHIFT BUF_REFCOUNT_BITS +#define BUF_FLAG_MASK (((1U << BUF_FLAG_BITS) - 1) << (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS)) /* Get refcount and usagecount from buffer state */ #define BUF_STATE_GET_REFCOUNT(state) ((state) & BUF_REFCOUNT_MASK) @@ -77,6 +84,9 @@ */ #define BM_MAX_USAGE_COUNT 5 +StaticAssertDecl(BM_MAX_USAGE_COUNT < (1 << BUF_USAGECOUNT_BITS), + "BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits"); + /* * Buffer tag identifies which disk block the buffer contains. * -- 2.48.1.76.g4e746b1a31.dirty
>From e01431cfa6559536c58aab1df386e9246618d95c Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Fri, 21 Feb 2025 13:26:57 -0500 Subject: [PATCH v2 4/4] bufmgr: Assert that MAX_BACKENDS is compatible with BufferDesc.state So far the dependency was documented in the comment above MAX_BACKENDS, but not checked. Discussion: https://postgr.es/m/CA+COZaBO_s3LfALq=b+hcbhfsoegiapvjrracce4vp9m7cj...@mail.gmail.com --- src/include/storage/buf_internals.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index d830d5c9841..8b32fb108b0 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -21,6 +21,7 @@ #include "storage/bufmgr.h" #include "storage/condition_variable.h" #include "storage/lwlock.h" +#include "storage/procnumber.h" #include "storage/shmem.h" #include "storage/smgr.h" #include "storage/spin.h" @@ -86,6 +87,8 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32, StaticAssertDecl(BM_MAX_USAGE_COUNT < (1 << BUF_USAGECOUNT_BITS), "BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits"); +StaticAssertDecl(MAX_BACKENDS_BITS <= BUF_REFCOUNT_BITS, + "MAX_BACKENDS_BITS needs to be <= BUF_REFCOUNT_BITS"); /* * Buffer tag identifies which disk block the buffer contains. -- 2.48.1.76.g4e746b1a31.dirty
