Hi Soumyadeep and Ashwin, Thanks for looking!
On Sun, Jul 18, 2021 at 6:58 AM Soumyadeep Chakraborty <soumyadeep2...@gmail.com> wrote: > You might have missed a spot to initialize SERIALIZABLE_XACT->pgprocno in > InitPredicateLocks(), so: > > + PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO; The magic OldCommittedSxact shouldn't be the target of a "signal", but this is definitely tidier. Thanks. > Slightly tangential: we should add a comment to PGPROC.pgprocno, for more > immediate understandability: > > + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */ I wonder why we need this member anyway, when you can compute it from the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs) or something like that? Kinda wonder why we don't use GetPGProcByNumber() in more places instead of open-coding access to ProcGlobal->allProcs, too... > 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. SERIALIZABLEXACT objects can live longer than the backends that created them. They hang around to sabotage other transactions' plans, depending on what else they overlapped with before they committed. With that change, the pg_locks view might show the pid of some unrelated session that moves into the same PGPROC. It's only an "informational" pid, and pids are imperfect information anyway because (1) they are themselves recycled, and (2) they won't be interesting in a hypothetical multi-threaded future. One solution would be to hide the pids from the view after the backend disconnects (somehow -- add a generation number?), but they're also still kinda useful, despite the weaknesses. I wonder what the ideal way would be to refer to sessions, anyway, including those that are no longer active; perhaps we could invent a new "session ID" concept.
From 4d5787efc2d13126476a371219cb16f7ec69e41b Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 11 Mar 2021 23:09:11 +1300 Subject: [PATCH v3] Optimize ProcSendSignal(). Instead of referring to target backends by pid, use pgprocno. This means that we don't have to scan the ProcArray, and we can drop some special case code for dealing with the startup process. Discussion: https://postgr.es/m/CA%2BhUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD%3Dq9HmwsfRRb-w%40mail.gmail.com Reviewed-by: Soumyadeep Chakraborty <soumyadeep2...@gmail.com> Reviewed-by: Ashwin Agrawal <ashwins...@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 | 6 ++- src/backend/storage/lmgr/proc.c | 49 ++--------------------- src/backend/utils/adt/lockfuncs.c | 1 + src/include/storage/buf_internals.h | 2 +- src/include/storage/predicate_internals.h | 1 + src/include/storage/proc.h | 6 +-- 9 files changed, 19 insertions(+), 60 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 2ee9515139..c2950a7c06 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 a299be1043..b9a83c0e9b 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 86ef607ff3..26122418fa 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 56267bdc3c..4f4d5b0d20 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -1277,6 +1277,7 @@ InitPredicateLocks(void) 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; @@ -1876,6 +1877,7 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot, sxact->finishedBefore = InvalidTransactionId; sxact->xmin = snapshot->xmin; sxact->pid = MyProcPid; + sxact->pgprocno = MyProc->pgprocno; SHMQueueInit(&(sxact->predicateLocks)); SHMQueueElemInit(&(sxact->finishedLink)); sxact->flags = 0; @@ -3669,7 +3671,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe) */ if (SxactIsDeferrableWaiting(roXact) && (SxactIsROUnsafe(roXact) || SxactIsROSafe(roXact))) - ProcSendSignal(roXact->pid); + ProcSendSignal(roXact->pgprocno); possibleUnsafeConflict = nextConflict; } @@ -5006,6 +5008,7 @@ PostPrepare_PredicateLocks(TransactionId xid) Assert(SxactIsPrepared(MySerializableXact)); MySerializableXact->pid = 0; + MySerializableXact->pgprocno = INVALID_PGPROCNO; hash_destroy(LocalPredicateLockHash); LocalPredicateLockHash = NULL; @@ -5081,6 +5084,7 @@ predicatelock_twophase_recover(TransactionId xid, uint16 info, 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 2575ea1ca0..a8934fdae8 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 5dc0a5882c..020f76e431 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" diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 33fcaf5c9a..9b634616e4 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 104f560d38..f154b3c3b8 100644 --- a/src/include/storage/predicate_internals.h +++ b/src/include/storage/predicate_internals.h @@ -113,6 +113,7 @@ typedef struct SERIALIZABLEXACT 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 be67d8a861..7c1e2efe30 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -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.30.2
