Hi Thomas, You might have missed a spot to initialize SERIALIZABLE_XACT->pgprocno in InitPredicateLocks(), so:
+ PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO; Slightly tangential: we should add a comment to PGPROC.pgprocno, for more immediate understandability: + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */ Also, why don't we take the opportunity to get rid of SERIALIZABLEXACT->pid? We took a stab. Attached is v2 of your patch with these changes. Regards, Ashwin and Deep
From 08d5ab9f498140e1c3a8f6f00d1acd25b3fb8ba0 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@gmail.com> Date: Thu, 11 Mar 2021 23:09:11 +1300 Subject: [PATCH v2 1/1] Optimize ProcSendSignal(). Instead of referring to target processes by pid, use pgprocno so that we don't have to scan the ProcArray and keep track of the startup process. Discussion: https://postgr.es/m/CA%2BhUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD%3Dq9HmwsfRRb-w%40mail.gmail.com --- src/backend/access/transam/xlog.c | 1 - src/backend/storage/buffer/buf_init.c | 3 +- src/backend/storage/buffer/bufmgr.c | 10 ++--- src/backend/storage/lmgr/predicate.c | 23 ++++++----- src/backend/storage/lmgr/proc.c | 49 ++--------------------- src/backend/utils/adt/lockfuncs.c | 8 +++- src/include/storage/buf_internals.h | 2 +- src/include/storage/predicate_internals.h | 2 +- src/include/storage/proc.h | 8 +--- 9 files changed, 34 insertions(+), 72 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index c7c928f50bd..2bbeaaf1b90 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7309,7 +7309,6 @@ StartupXLOG(void) */ if (ArchiveRecoveryRequested && IsUnderPostmaster) { - PublishStartupProcessInformation(); EnableSyncRequestForwarding(); SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED); bgwriterLaunched = true; diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c index a299be10430..b9a83c0e9b9 100644 --- a/src/backend/storage/buffer/buf_init.c +++ b/src/backend/storage/buffer/buf_init.c @@ -16,6 +16,7 @@ #include "storage/buf_internals.h" #include "storage/bufmgr.h" +#include "storage/proc.h" BufferDescPadded *BufferDescriptors; char *BufferBlocks; @@ -118,7 +119,7 @@ InitBufferPool(void) CLEAR_BUFFERTAG(buf->tag); pg_atomic_init_u32(&buf->state, 0); - buf->wait_backend_pid = 0; + buf->wait_backend_pgprocno = INVALID_PGPROCNO; buf->buf_id = i; diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 86ef607ff38..26122418faf 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1890,11 +1890,11 @@ UnpinBuffer(BufferDesc *buf, bool fixOwner) BUF_STATE_GET_REFCOUNT(buf_state) == 1) { /* we just released the last pin other than the waiter's */ - int wait_backend_pid = buf->wait_backend_pid; + int wait_backend_pgprocno = buf->wait_backend_pgprocno; buf_state &= ~BM_PIN_COUNT_WAITER; UnlockBufHdr(buf, buf_state); - ProcSendSignal(wait_backend_pid); + ProcSendSignal(wait_backend_pgprocno); } else UnlockBufHdr(buf, buf_state); @@ -3995,7 +3995,7 @@ UnlockBuffers(void) * got a cancel/die interrupt before getting the signal. */ if ((buf_state & BM_PIN_COUNT_WAITER) != 0 && - buf->wait_backend_pid == MyProcPid) + buf->wait_backend_pgprocno == MyProc->pgprocno) buf_state &= ~BM_PIN_COUNT_WAITER; UnlockBufHdr(buf, buf_state); @@ -4131,7 +4131,7 @@ LockBufferForCleanup(Buffer buffer) LockBuffer(buffer, BUFFER_LOCK_UNLOCK); elog(ERROR, "multiple backends attempting to wait for pincount 1"); } - bufHdr->wait_backend_pid = MyProcPid; + bufHdr->wait_backend_pgprocno = MyProc->pgprocno; PinCountWaitBuf = bufHdr; buf_state |= BM_PIN_COUNT_WAITER; UnlockBufHdr(bufHdr, buf_state); @@ -4202,7 +4202,7 @@ LockBufferForCleanup(Buffer buffer) */ buf_state = LockBufHdr(bufHdr); if ((buf_state & BM_PIN_COUNT_WAITER) != 0 && - bufHdr->wait_backend_pid == MyProcPid) + bufHdr->wait_backend_pgprocno == MyProc->pgprocno) buf_state &= ~BM_PIN_COUNT_WAITER; UnlockBufHdr(bufHdr, buf_state); diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index d493aeef0fc..d76632bb56a 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -1276,7 +1276,7 @@ InitPredicateLocks(void) PredXact->OldCommittedSxact->finishedBefore = InvalidTransactionId; PredXact->OldCommittedSxact->xmin = InvalidTransactionId; PredXact->OldCommittedSxact->flags = SXACT_FLAG_COMMITTED; - PredXact->OldCommittedSxact->pid = 0; + PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO; } /* This never changes, so let's keep a local copy. */ OldCommittedSxact = PredXact->OldCommittedSxact; @@ -1635,8 +1635,12 @@ GetSafeSnapshotBlockingPids(int blocked_pid, int *output, int output_size) /* Find blocked_pid's SERIALIZABLEXACT by linear search. */ for (sxact = FirstPredXact(); sxact != NULL; sxact = NextPredXact(sxact)) { - if (sxact->pid == blocked_pid) - break; + if (sxact->pgprocno != INVALID_PGPROCNO) + { + PGPROC *proc = GetPGProcByNumber(sxact->pgprocno); + if (proc->pid == blocked_pid) + break; + } } /* Did we find it, and is it currently waiting in GetSafeSnapshot? */ @@ -1652,7 +1656,8 @@ GetSafeSnapshotBlockingPids(int blocked_pid, int *output, int output_size) while (possibleUnsafeConflict != NULL && num_written < output_size) { - output[num_written++] = possibleUnsafeConflict->sxactOut->pid; + PGPROC *outputProc = GetPGProcByNumber(possibleUnsafeConflict->sxactOut->pgprocno); + output[num_written++] = outputProc->pid; possibleUnsafeConflict = (RWConflict) SHMQueueNext(&sxact->possibleUnsafeConflicts, &possibleUnsafeConflict->inLink, @@ -1875,7 +1880,7 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot, sxact->topXid = GetTopTransactionIdIfAny(); sxact->finishedBefore = InvalidTransactionId; sxact->xmin = snapshot->xmin; - sxact->pid = MyProcPid; + sxact->pgprocno = MyProc->pgprocno; SHMQueueInit(&(sxact->predicateLocks)); SHMQueueElemInit(&(sxact->finishedLink)); sxact->flags = 0; @@ -3441,7 +3446,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe) || !SxactIsRolledBack(MySerializableXact)); /* may not be serializable during COMMIT/ROLLBACK PREPARED */ - Assert(MySerializableXact->pid == 0 || IsolationIsSerializable()); + Assert(MySerializableXact->pgprocno == INVALID_PGPROCNO || IsolationIsSerializable()); /* We'd better not already be on the cleanup list. */ Assert(!SxactIsOnFinishedList(MySerializableXact)); @@ -3669,7 +3674,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe) */ if (SxactIsDeferrableWaiting(roXact) && (SxactIsROUnsafe(roXact) || SxactIsROSafe(roXact))) - ProcSendSignal(roXact->pid); + ProcSendSignal(roXact->pgprocno); possibleUnsafeConflict = nextConflict; } @@ -5005,7 +5010,7 @@ PostPrepare_PredicateLocks(TransactionId xid) Assert(SxactIsPrepared(MySerializableXact)); - MySerializableXact->pid = 0; + MySerializableXact->pgprocno = INVALID_PGPROCNO; hash_destroy(LocalPredicateLockHash); LocalPredicateLockHash = NULL; @@ -5080,7 +5085,7 @@ predicatelock_twophase_recover(TransactionId xid, uint16 info, /* vxid for a prepared xact is InvalidBackendId/xid; no pid */ sxact->vxid.backendId = InvalidBackendId; sxact->vxid.localTransactionId = (LocalTransactionId) xid; - sxact->pid = 0; + sxact->pgprocno = INVALID_PGPROCNO; /* a prepared xact hasn't committed yet */ sxact->prepareSeqNo = RecoverySerCommitSeqNo; diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 2575ea1ca0d..a8934fdae8b 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -177,8 +177,6 @@ InitProcGlobal(void) ProcGlobal->autovacFreeProcs = NULL; ProcGlobal->bgworkerFreeProcs = NULL; ProcGlobal->walsenderFreeProcs = NULL; - ProcGlobal->startupProc = NULL; - ProcGlobal->startupProcPid = 0; ProcGlobal->startupBufferPinWaitBufId = -1; ProcGlobal->walwriterLatch = NULL; ProcGlobal->checkpointerLatch = NULL; @@ -624,21 +622,6 @@ InitAuxiliaryProcess(void) on_shmem_exit(AuxiliaryProcKill, Int32GetDatum(proctype)); } -/* - * Record the PID and PGPROC structures for the Startup process, for use in - * ProcSendSignal(). See comments there for further explanation. - */ -void -PublishStartupProcessInformation(void) -{ - SpinLockAcquire(ProcStructLock); - - ProcGlobal->startupProc = MyProc; - ProcGlobal->startupProcPid = MyProcPid; - - SpinLockRelease(ProcStructLock); -} - /* * Used from bufmgr to share the value of the buffer that Startup waits on, * or to reset the value to "not waiting" (-1). This allows processing @@ -1903,38 +1886,12 @@ ProcWaitForSignal(uint32 wait_event_info) } /* - * ProcSendSignal - send a signal to a backend identified by PID + * ProcSendSignal - send a signal to a backend identified by pgprocno */ void -ProcSendSignal(int pid) +ProcSendSignal(int pgprocno) { - PGPROC *proc = NULL; - - if (RecoveryInProgress()) - { - SpinLockAcquire(ProcStructLock); - - /* - * Check to see whether it is the Startup process we wish to signal. - * This call is made by the buffer manager when it wishes to wake up a - * process that has been waiting for a pin in so it can obtain a - * cleanup lock using LockBufferForCleanup(). Startup is not a normal - * backend, so BackendPidGetProc() will not return any pid at all. So - * we remember the information for this special case. - */ - if (pid == ProcGlobal->startupProcPid) - proc = ProcGlobal->startupProc; - - SpinLockRelease(ProcStructLock); - } - - if (proc == NULL) - proc = BackendPidGetProc(pid); - - if (proc != NULL) - { - SetLatch(&proc->procLatch); - } + SetLatch(&ProcGlobal->allProcs[pgprocno].procLatch); } /* diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c index 085fec3ea20..695693c33fd 100644 --- a/src/backend/utils/adt/lockfuncs.c +++ b/src/backend/utils/adt/lockfuncs.c @@ -18,6 +18,7 @@ #include "funcapi.h" #include "miscadmin.h" #include "storage/predicate_internals.h" +#include "storage/proc.h" #include "utils/array.h" #include "utils/builtins.h" @@ -400,8 +401,11 @@ pg_lock_status(PG_FUNCTION_ARGS) /* lock holder */ values[10] = VXIDGetDatum(xact->vxid.backendId, xact->vxid.localTransactionId); - if (xact->pid != 0) - values[11] = Int32GetDatum(xact->pid); + if (xact->pgprocno != INVALID_PGPROCNO) + { + PGPROC *proc = GetPGProcByNumber(xact->pgprocno); + values[11] = Int32GetDatum(proc->pid); + } else nulls[11] = true; diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 33fcaf5c9a8..9b634616e4f 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -187,7 +187,7 @@ typedef struct BufferDesc /* state of the tag, containing flags, refcount and usagecount */ pg_atomic_uint32 state; - int wait_backend_pid; /* backend PID of pin-count waiter */ + int wait_backend_pgprocno; /* backend of pin-count waiter */ int freeNext; /* link in freelist chain */ LWLock content_lock; /* to lock access to buffer contents */ } BufferDesc; diff --git a/src/include/storage/predicate_internals.h b/src/include/storage/predicate_internals.h index 104f560d380..4f4b28a91d5 100644 --- a/src/include/storage/predicate_internals.h +++ b/src/include/storage/predicate_internals.h @@ -112,7 +112,7 @@ typedef struct SERIALIZABLEXACT * xids are before this. */ TransactionId xmin; /* the transaction's snapshot xmin */ uint32 flags; /* OR'd combination of values defined below */ - int pid; /* pid of associated process */ + int pgprocno; /* pgprocno of associated process */ } SERIALIZABLEXACT; #define SXACT_FLAG_COMMITTED 0x00000001 /* already committed */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index be67d8a8616..92137a8b3a2 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -147,7 +147,7 @@ struct PGPROC int pgxactoff; /* offset into various ProcGlobal->arrays with * data mirrored from this PGPROC */ - int pgprocno; + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */ /* These fields are zero while a backend is still starting up: */ BackendId backendId; /* This backend's backend ID (if assigned) */ @@ -352,9 +352,6 @@ typedef struct PROC_HDR Latch *checkpointerLatch; /* Current shared estimate of appropriate spins_per_delay value */ int spins_per_delay; - /* The proc of the Startup process, since not in ProcArray */ - PGPROC *startupProc; - int startupProcPid; /* Buffer id of the buffer that Startup process waits for pin on, or -1 */ int startupBufferPinWaitBufId; } PROC_HDR; @@ -395,7 +392,6 @@ extern void InitProcess(void); extern void InitProcessPhase2(void); extern void InitAuxiliaryProcess(void); -extern void PublishStartupProcessInformation(void); extern void SetStartupBufferPinWaitBufId(int bufid); extern int GetStartupBufferPinWaitBufId(void); @@ -411,7 +407,7 @@ extern bool IsWaitingForLock(void); extern void LockErrorCleanup(void); extern void ProcWaitForSignal(uint32 wait_event_info); -extern void ProcSendSignal(int pid); +extern void ProcSendSignal(int pgprocno); extern PGPROC *AuxiliaryPidGetProc(int pid); -- 2.25.1
