From eac38c347318a0ee074b25d487de66575a242e12 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 22 Mar 2025 12:50:38 +0100 Subject: [PATCH v20250324 4/6] review --- src/backend/storage/lmgr/predicate.c | 21 +++++--- src/backend/storage/lmgr/proc.c | 75 +++++++++++++++++++--------- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index dd66990335b..aeba84de786 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -1237,6 +1237,9 @@ PredicateLockShmemInit(void) { int i; + /* reset everything, both the header and the element */ + memset(PredXact, 0, requestSize); + dlist_init(&PredXact->availableList); dlist_init(&PredXact->activeList); PredXact->SxactGlobalXmin = InvalidTransactionId; @@ -1247,7 +1250,9 @@ PredicateLockShmemInit(void) PredXact->HavePartialClearedThrough = 0; PredXact->element = (SERIALIZABLEXACT *) ((char *) PredXact + PredXactListDataSize); /* Add all elements to available list, clean. */ - memset(PredXact->element, 0, requestSize); + /* XXX wasn't this actually wrong, considering requestSize is the whole + * shmem allocation, including PredXactListDataSize? */ + // memset(PredXact->element, 0, requestSize); for (i = 0; i < max_table_size; i++) { LWLockInitialize(&PredXact->element[i].perXactPredicateListLock, @@ -1300,21 +1305,25 @@ PredicateLockShmemInit(void) * probably OK. */ max_table_size *= 5; - requestSize = mul_size((Size) max_table_size, - RWConflictDataSize); + requestSize = RWConflictPoolHeaderDataSize + + mul_size((Size) max_table_size, + RWConflictDataSize); RWConflictPool = ShmemInitStruct("RWConflictPool", - RWConflictPoolHeaderDataSize + requestSize, + requestSize, &found); Assert(found == IsUnderPostmaster); if (!found) { int i; + /* clean everything, including the elements */ + memset(RWConflictPool, 0, requestSize); + dlist_init(&RWConflictPool->availableList); - RWConflictPool->element = (RWConflict) ((char *) RWConflictPool + RWConflictPoolHeaderDataSize); + RWConflictPool->element = (RWConflict) ((char *) RWConflictPool + + RWConflictPoolHeaderDataSize); /* Add all elements to available list, clean. */ - memset(RWConflictPool->element, 0, requestSize); for (i = 0; i < max_table_size; i++) { dlist_push_tail(&RWConflictPool->availableList, diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 65239d743da..4afd7cd42c3 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -88,7 +88,6 @@ static void RemoveProcFromArray(int code, Datum arg); static void ProcKill(int code, Datum arg); static void AuxiliaryProcKill(int code, Datum arg); static void CheckDeadLock(void); -static Size PGProcShmemSize(void); /* @@ -124,6 +123,27 @@ ProcGlobalShmemSize(void) return size; } +/* + * review: add comment, explaining the PG_CACHE_LINE_SIZE thing + * review: I'd even maybe split the PG_CACHE_LINE_SIZE thing into + * a separate commit, not to mix it with the "monitoring improvement" + */ +static Size +PGProcShmemSize(void) +{ + Size size; + uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts; + + size = TotalProcs * sizeof(PGPROC); + size = add_size(size, TotalProcs * sizeof(*ProcGlobal->xids)); + size = add_size(size, PG_CACHE_LINE_SIZE); + size = add_size(size, TotalProcs * sizeof(*ProcGlobal->subxidStates)); + size = add_size(size, PG_CACHE_LINE_SIZE); + size = add_size(size, TotalProcs * sizeof(*ProcGlobal->statusFlags)); + size = add_size(size, PG_CACHE_LINE_SIZE); + return size; +} + /* * Report number of semaphores needed by InitProcGlobal. */ @@ -177,6 +197,7 @@ InitProcGlobal(void) Size fpLockBitsSize, fpRelIdSize; Size requestSize; + char *ptr; /* Create the ProcGlobal shared structure */ ProcGlobal = (PROC_HDR *) @@ -208,7 +229,12 @@ InitProcGlobal(void) */ requestSize = PGProcShmemSize(); - procs = (PGPROC *) ShmemInitStruct("PGPROC structures", requestSize, &found); + ptr = ShmemInitStruct("PGPROC structures", + requestSize, + &found); + + procs = (PGPROC *) ptr; + ptr += TotalProcs * sizeof(PGPROC); MemSet(procs, 0, TotalProcs * sizeof(PGPROC)); ProcGlobal->allProcs = procs; @@ -221,14 +247,27 @@ InitProcGlobal(void) * * XXX: It might make sense to increase padding for these arrays, given * how hotly they are accessed. + * + * review: does the padding comment still make sense with PG_CACHE_LINE_SIZE? + * presumably that's the padding mentioned by the comment? + * + * review: those lines are too long / not comprehensible, let's define some + * macros to calculate stuff? */ - ProcGlobal->xids = - (TransactionId *) ((char *) procs + TotalProcs * sizeof(PGPROC)); + ProcGlobal->xids = (TransactionId *) ptr; MemSet(ProcGlobal->xids, 0, TotalProcs * sizeof(*ProcGlobal->xids)); - ProcGlobal->subxidStates = (XidCacheStatus *) ((char *) ProcGlobal->xids + TotalProcs * sizeof(*ProcGlobal->xids) + PG_CACHE_LINE_SIZE); + ptr += TotalProcs * sizeof(*ProcGlobal->xids) + PG_CACHE_LINE_SIZE; + + ProcGlobal->subxidStates = (XidCacheStatus *) ptr; MemSet(ProcGlobal->subxidStates, 0, TotalProcs * sizeof(*ProcGlobal->subxidStates)); - ProcGlobal->statusFlags = (uint8 *) ((char *) ProcGlobal->subxidStates + TotalProcs * sizeof(*ProcGlobal->subxidStates) + PG_CACHE_LINE_SIZE); + ptr += TotalProcs * sizeof(*ProcGlobal->subxidStates) + PG_CACHE_LINE_SIZE; + + ProcGlobal->statusFlags = (uint8 *) ptr; MemSet(ProcGlobal->statusFlags, 0, TotalProcs * sizeof(*ProcGlobal->statusFlags)); + ptr += TotalProcs * sizeof(*ProcGlobal->statusFlags) + PG_CACHE_LINE_SIZE; + + /* make sure wer didn't overflow */ + Assert((ptr > (char *) procs) && (ptr <= (char *) procs + requestSize)); /* * Allocate arrays for fast-path locks. Those are variable-length, so @@ -238,7 +277,9 @@ InitProcGlobal(void) fpLockBitsSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(uint64)); fpRelIdSize = MAXALIGN(FastPathLockSlotsPerBackend() * sizeof(Oid)); - fpPtr = ShmemInitStruct("Fast path lock arrays", TotalProcs * (fpLockBitsSize + fpRelIdSize), &found); + fpPtr = ShmemInitStruct("Fast path lock arrays", + TotalProcs * (fpLockBitsSize + fpRelIdSize), + &found); MemSet(fpPtr, 0, TotalProcs * (fpLockBitsSize + fpRelIdSize)); /* For asserts checking we did not overflow. */ @@ -335,26 +376,12 @@ InitProcGlobal(void) PreparedXactProcs = &procs[MaxBackends + NUM_AUXILIARY_PROCS]; /* Create ProcStructLock spinlock, too */ - ProcStructLock = (slock_t *) ShmemInitStruct("ProcStructLock spinlock", sizeof(slock_t), &found); + ProcStructLock = (slock_t *) ShmemInitStruct("ProcStructLock spinlock", + sizeof(slock_t), + &found); SpinLockInit(ProcStructLock); } -static Size -PGProcShmemSize(void) -{ - Size size; - uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts; - - size = TotalProcs * sizeof(PGPROC); - size = add_size(size, TotalProcs * sizeof(*ProcGlobal->xids)); - size = add_size(size, PG_CACHE_LINE_SIZE); - size = add_size(size, TotalProcs * sizeof(*ProcGlobal->subxidStates)); - size = add_size(size, PG_CACHE_LINE_SIZE); - size = add_size(size, TotalProcs * sizeof(*ProcGlobal->statusFlags)); - size = add_size(size, PG_CACHE_LINE_SIZE); - return size; -} - /* * InitProcess -- initialize a per-process PGPROC entry for this backend */ -- 2.49.0