A micro-optimisation for ProcSendSignal()
Hi,
ProcSendSignal(pid) searches the ProcArray for the given pid and then
sets that backend's procLatch. It has only two users: UnpinBuffer()
and ReleasePredicateLocks(). In both cases, we could just as easily
have recorded the pgprocno instead, avoiding the locking and the
searching. We'd also be able to drop some special book-keeping for
the startup process, whose pid can't be found via the ProcArray.
A related idea, saving space in BufferDesc but having to do slightly
more expensive work, would be for UnpinBuffer() to reuse the new
condition variable instead of ProcSendSignal().
Attachments:
0001-Optimize-ProcSendSignal.patchtext/x-patch; charset=US-ASCII; name=0001-Optimize-ProcSendSignal.patchDownload
From b2f3bf47e11f572a32f24945d10ffb9a7bc47b1f 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] 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.
---
src/backend/access/transam/xlog.c | 1 -
src/backend/storage/buffer/buf_init.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 10 ++---
src/backend/storage/lmgr/predicate.c | 5 ++-
src/backend/storage/lmgr/proc.c | 49 ++---------------------
src/include/storage/buf_internals.h | 2 +-
src/include/storage/predicate_internals.h | 1 +
src/include/storage/proc.h | 6 +--
8 files changed, 16 insertions(+), 60 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 18af3d4120..5ff9bbda27 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7225,7 +7225,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..9ae17cf4da 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -118,7 +118,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 4c1d5eceb4..84b3669d8d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1818,11 +1818,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);
@@ -3923,7 +3923,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);
@@ -4059,7 +4059,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);
@@ -4130,7 +4130,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 d493aeef0f..c151e0c30c 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1876,6 +1876,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 +3670,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
*/
if (SxactIsDeferrableWaiting(roXact) &&
(SxactIsROUnsafe(roXact) || SxactIsROSafe(roXact)))
- ProcSendSignal(roXact->pid);
+ ProcSendSignal(roXact->pgprocno);
possibleUnsafeConflict = nextConflict;
}
@@ -5006,6 +5007,7 @@ PostPrepare_PredicateLocks(TransactionId xid)
Assert(SxactIsPrepared(MySerializableXact));
MySerializableXact->pid = 0;
+ MySerializableXact->pgprocno = INVALID_PGPROCNO;
hash_destroy(LocalPredicateLockHash);
LocalPredicateLockHash = NULL;
@@ -5081,6 +5083,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 897045ee27..b7c10c73d9 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;
@@ -618,21 +616,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
@@ -1894,38 +1877,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/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 a777cb64a1..d298ab8a14 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.1
On Fri, Mar 12, 2021 at 12:31 AM Thomas Munro <thomas.munro@gmail.com> wrote:
ProcSendSignal(pid) searches the ProcArray for the given pid and then
sets that backend's procLatch. It has only two users: UnpinBuffer()
and ReleasePredicateLocks(). In both cases, we could just as easily
have recorded the pgprocno instead, avoiding the locking and the
searching. We'd also be able to drop some special book-keeping for
the startup process, whose pid can't be found via the ProcArray.
Rebased.
Attachments:
v2-0001-Optimize-ProcSendSignal.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Optimize-ProcSendSignal.patchDownload
From 5c2d78110d5efb61f40cf741948a4b247c0bddaf 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] 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 | 5 ++-
src/backend/storage/lmgr/proc.c | 49 ++---------------------
src/include/storage/buf_internals.h | 2 +-
src/include/storage/predicate_internals.h | 1 +
src/include/storage/proc.h | 6 +--
8 files changed, 17 insertions(+), 60 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 441a9124cd..6a394f3583 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7281,7 +7281,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 4b296a22c4..ab3506f05b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1896,11 +1896,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);
@@ -4007,7 +4007,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);
@@ -4143,7 +4143,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);
@@ -4214,7 +4214,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 d493aeef0f..c151e0c30c 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1876,6 +1876,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 +3670,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
*/
if (SxactIsDeferrableWaiting(roXact) &&
(SxactIsROUnsafe(roXact) || SxactIsROSafe(roXact)))
- ProcSendSignal(roXact->pid);
+ ProcSendSignal(roXact->pgprocno);
possibleUnsafeConflict = nextConflict;
}
@@ -5006,6 +5007,7 @@ PostPrepare_PredicateLocks(TransactionId xid)
Assert(SxactIsPrepared(MySerializableXact));
MySerializableXact->pid = 0;
+ MySerializableXact->pgprocno = INVALID_PGPROCNO;
hash_destroy(LocalPredicateLockHash);
LocalPredicateLockHash = NULL;
@@ -5081,6 +5083,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/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
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
Attachments:
v2-0001-Optimize-ProcSendSignal.patchapplication/x-patch; name=v2-0001-Optimize-ProcSendSignal.patchDownload
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
Hi Soumyadeep and Ashwin,
Thanks for looking!
On Sun, Jul 18, 2021 at 6:58 AM Soumyadeep Chakraborty
<soumyadeep2007@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.
Attachments:
v3-0001-Optimize-ProcSendSignal.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Optimize-ProcSendSignal.patchDownload
From 4d5787efc2d13126476a371219cb16f7ec69e41b 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 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 <soumyadeep2007@gmail.com>
Reviewed-by: Ashwin Agrawal <ashwinstar@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
HI Thomas,
On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
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...
I tried this out. See attached v4 of your patch with these changes.
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.
I see.
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.
Updating the pg_locks view:
Yes, the pids may be valuable for future debugging/audit purposes. Also,
systems where pid_max is high enough to not see wraparound, will have
pids that are not recycled. I would lean towards showing the pid even
after the backend has exited.
Perhaps we could overload the stored pid to be negated (i.e. a backend
with pid 20000 will become -20000) to indicate that the pid belongs to
a backend that has exited. Additionally, we could introduce a boolean
field in pg_locks "backendisalive", so that the end user doesn't have
to reason about negative pids.
Session ID:
Interesting, Greenplum uses the concept of session ID pervasively. Being
a distributed database, the session ID helps to tie individual backends
on each PG instance to the same session. The session ID of course comes
with its share of bookkeeping:
* These IDs are incrementally dished out with a counter
(with pg_atomic_add_fetch_u32), in increments of 1, on the Greenplum
coordinator PG instance in InitProcess.
* The counter is a part of ProcGlobal and itself is initialized to 0 in
InitProcGlobal, which means that session IDs are reset on cluster
restart.
* The sessionID tied to each proceess is maintained in PGPROC.
* The sessionID finds its way into PgBackendStatus, which is further
reported with pg_stat_get_activity.
A session ID seems a bit heavy just to indicate whether a backend has
exited.
Regards,
Soumyadeep
Attachments:
v4-0001-Optimize-ProcSendSignal.patchtext/x-patch; charset=US-ASCII; name=v4-0001-Optimize-ProcSendSignal.patchDownload
From c964a977ca41c057737ca34420e002dc85663eb0 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 v4 1/1] 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 <soumyadeep2007@gmail.com>
Reviewed-by: Ashwin Agrawal <ashwinstar@gmail.com>
---
src/backend/access/transam/clog.c | 2 +-
src/backend/access/transam/twophase.c | 3 +-
src/backend/access/transam/xlog.c | 3 +-
src/backend/postmaster/bgwriter.c | 2 +-
src/backend/postmaster/pgarch.c | 2 +-
src/backend/storage/buffer/buf_init.c | 3 +-
src/backend/storage/buffer/bufmgr.c | 10 ++--
src/backend/storage/ipc/procarray.c | 6 +--
src/backend/storage/lmgr/condition_variable.c | 12 ++---
src/backend/storage/lmgr/lwlock.c | 6 +--
src/backend/storage/lmgr/predicate.c | 6 ++-
src/backend/storage/lmgr/proc.c | 50 ++-----------------
src/backend/utils/adt/lockfuncs.c | 1 +
src/include/storage/buf_internals.h | 2 +-
src/include/storage/lock.h | 2 +-
src/include/storage/predicate_internals.h | 1 +
src/include/storage/proc.h | 12 ++---
17 files changed, 42 insertions(+), 81 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 3ea16a270a8..d0557f6c555 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -464,7 +464,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
&nextidx,
- (uint32) proc->pgprocno))
+ (uint32) GetPGProcNumber(proc)))
break;
}
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6d3efb49a40..0fe8a59cf06 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -279,7 +279,7 @@ TwoPhaseShmemInit(void)
TwoPhaseState->freeGXacts = &gxacts[i];
/* associate it with a PGPROC assigned by InitProcGlobal */
- gxacts[i].pgprocno = PreparedXactProcs[i].pgprocno;
+ gxacts[i].pgprocno = GetPGProcNumber(&PreparedXactProcs[i]);
/*
* Assign a unique ID for each dummy proc, so that the range of
@@ -456,7 +456,6 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
/* Initialize the PGPROC entry */
MemSet(proc, 0, sizeof(PGPROC));
- proc->pgprocno = gxact->pgprocno;
SHMQueueElemInit(&(proc->links));
proc->waitStatus = PROC_WAIT_STATUS_OK;
/* We set up the gxact's VXID as InvalidBackendId/XID */
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2ee95151392..a0d2e108f6c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1687,7 +1687,7 @@ WALInsertLockAcquire(void)
static int lockToTry = -1;
if (lockToTry == -1)
- lockToTry = MyProc->pgprocno % NUM_XLOGINSERT_LOCKS;
+ lockToTry = GetPGProcNumber(MyProc) % NUM_XLOGINSERT_LOCKS;
MyLockNo = lockToTry;
/*
@@ -7309,7 +7309,6 @@ StartupXLOG(void)
*/
if (ArchiveRecoveryRequested && IsUnderPostmaster)
{
- PublishStartupProcessInformation();
EnableSyncRequestForwarding();
SendPostmasterSignal(PMSIGNAL_RECOVERY_STARTED);
bgwriterLaunched = true;
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 715d5195bb6..086d6346b6e 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -336,7 +336,7 @@ BackgroundWriterMain(void)
if (rc == WL_TIMEOUT && can_hibernate && prev_hibernate)
{
/* Ask for notification at next buffer allocation */
- StrategyNotifyBgWriter(MyProc->pgprocno);
+ StrategyNotifyBgWriter(GetPGProcNumber(MyProc));
/* Sleep ... */
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 74a7d7c4d0a..68de7ee3b8c 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -196,7 +196,7 @@ PgArchiverMain(void)
* Advertise our pgprocno so that backends can use our latch to wake us up
* while we're sleeping.
*/
- PgArch->pgprocno = MyProc->pgprocno;
+ PgArch->pgprocno = GetPGProcNumber(MyProc);
pgarch_MainLoop();
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..e3621cb8344 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 == GetPGProcNumber(MyProc))
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 = GetPGProcNumber(MyProc);
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 == GetPGProcNumber(MyProc))
buf_state &= ~BM_PIN_COUNT_WAITER;
UnlockBufHdr(bufHdr, buf_state);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 4c91e721d0c..6ca56933451 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -481,7 +481,7 @@ ProcArrayAdd(PGPROC *proc)
Assert(allProcs[procno].pgxactoff == index);
/* If we have found our right position in the array, break */
- if (arrayP->pgprocnos[index] > proc->pgprocno)
+ if (arrayP->pgprocnos[index] > GetPGProcNumber(proc))
break;
}
@@ -499,7 +499,7 @@ ProcArrayAdd(PGPROC *proc)
&ProcGlobal->statusFlags[index],
movecount * sizeof(*ProcGlobal->statusFlags));
- arrayP->pgprocnos[index] = proc->pgprocno;
+ arrayP->pgprocnos[index] = GetPGProcNumber(proc);
proc->pgxactoff = index;
ProcGlobal->xids[index] = proc->xid;
ProcGlobal->subxidStates[index] = proc->subxidStatus;
@@ -778,7 +778,7 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
if (pg_atomic_compare_exchange_u32(&procglobal->procArrayGroupFirst,
&nextidx,
- (uint32) proc->pgprocno))
+ (uint32) GetPGProcNumber(proc)))
break;
}
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 80d70c154cf..d0728d17bd3 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -57,7 +57,7 @@ ConditionVariableInit(ConditionVariable *cv)
void
ConditionVariablePrepareToSleep(ConditionVariable *cv)
{
- int pgprocno = MyProc->pgprocno;
+ int pgprocno = GetPGProcNumber(MyProc);
/*
* If some other sleep is already prepared, cancel it; this is necessary
@@ -181,10 +181,10 @@ ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
* guarantee not to return spuriously, we'll avoid this obvious case.
*/
SpinLockAcquire(&cv->mutex);
- if (!proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
+ if (!proclist_contains(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink))
{
done = true;
- proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
+ proclist_push_tail(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink);
}
SpinLockRelease(&cv->mutex);
@@ -234,8 +234,8 @@ ConditionVariableCancelSleep(void)
return;
SpinLockAcquire(&cv->mutex);
- if (proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
- proclist_delete(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
+ if (proclist_contains(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink))
+ proclist_delete(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink);
else
signaled = true;
SpinLockRelease(&cv->mutex);
@@ -285,7 +285,7 @@ ConditionVariableSignal(ConditionVariable *cv)
void
ConditionVariableBroadcast(ConditionVariable *cv)
{
- int pgprocno = MyProc->pgprocno;
+ int pgprocno = GetPGProcNumber(MyProc);
PGPROC *proc = NULL;
bool have_sentinel = false;
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 862097352bb..e3cef6700aa 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1080,9 +1080,9 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
/* LW_WAIT_UNTIL_FREE waiters are always at the front of the queue */
if (mode == LW_WAIT_UNTIL_FREE)
- proclist_push_head(&lock->waiters, MyProc->pgprocno, lwWaitLink);
+ proclist_push_head(&lock->waiters, GetPGProcNumber(MyProc), lwWaitLink);
else
- proclist_push_tail(&lock->waiters, MyProc->pgprocno, lwWaitLink);
+ proclist_push_tail(&lock->waiters, GetPGProcNumber(MyProc), lwWaitLink);
/* Can release the mutex now */
LWLockWaitListUnlock(lock);
@@ -1122,7 +1122,7 @@ LWLockDequeueSelf(LWLock *lock)
*/
proclist_foreach_modify(iter, &lock->waiters, lwWaitLink)
{
- if (iter.cur == MyProc->pgprocno)
+ if (iter.cur == GetPGProcNumber(MyProc))
{
found = true;
proclist_delete(&lock->waiters, iter.cur, lwWaitLink);
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 56267bdc3ce..804045017dd 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 = GetPGProcNumber(MyProc);
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 2575ea1ca0d..59b93688476 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;
@@ -229,7 +227,6 @@ InitProcGlobal(void)
InitSharedLatch(&(procs[i].procLatch));
LWLockInitialize(&(procs[i].fpInfoLock), LWTRANCHE_LOCK_FASTPATH);
}
- procs[i].pgprocno = i;
/*
* Newly created PGPROCs for normal backends, autovacuum and bgworkers
@@ -624,21 +621,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 +1885,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 5dc0a5882cf..020f76e4316 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 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/lock.h b/src/include/storage/lock.h
index 9b2a421c32c..54045996663 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -531,7 +531,7 @@ typedef enum
* used for a given lock group is determined by the group leader's pgprocno.
*/
#define LockHashPartitionLockByProc(leader_pgproc) \
- LockHashPartitionLock((leader_pgproc)->pgprocno)
+ LockHashPartitionLock(GetPGProcNumber((leader_pgproc)))
/*
* function prototypes
diff --git a/src/include/storage/predicate_internals.h b/src/include/storage/predicate_internals.h
index 104f560d380..f154b3c3b85 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 be67d8a8616..7673726d9b8 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -147,7 +147,6 @@ struct PGPROC
int pgxactoff; /* offset into various ProcGlobal->arrays with
* data mirrored from this PGPROC */
- int pgprocno;
/* These fields are zero while a backend is still starting up: */
BackendId backendId; /* This backend's backend ID (if assigned) */
@@ -311,6 +310,9 @@ extern PGDLLIMPORT PGPROC *MyProc;
* When entering a PGPROC for 2PC transactions with ProcArrayAdd(), the data
* in the dense arrays is initialized from the PGPROC while it already holds
* ProcArrayLock.
+ *
+ * Shared memory data structures needing to refer to a PGPROC in the procarray
+ * can use its index to do so. See GetPGProcNumber below.
*/
typedef struct PROC_HDR
{
@@ -352,9 +354,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;
@@ -365,6 +364,8 @@ extern PGPROC *PreparedXactProcs;
/* Accessor for PGPROC given a pgprocno. */
#define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)])
+/* Calculate position of PGPROC in procarray. */
+#define GetPGProcNumber(proc) ((proc) - ProcGlobal->allProcs)
/*
* We set aside some extra PGPROC structures for auxiliary processes,
@@ -395,7 +396,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 +411,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
Hi Soumyadeep,
On Sat, Jul 24, 2021 at 5:26 PM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:
On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
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...I tried this out. See attached v4 of your patch with these changes.
I like it. I've moved these changes to a separate patch, 0002, for
separate commit. I tweaked a couple of comments (it's not a position
in the "procarray", well it's a position stored in the procarray, but
that's confusing; I also found a stray comment about leader->pgprocno
that is obsoleted by this change). Does anyone have objections to
this?
I was going to commit the earlier change this morning, but then I read [1]/messages/by-id/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de.
New idea. Instead of adding pgprocno to SERIALIZABLEXACT (which we
should really be trying to shrink, not grow), let's look it up by
vxid->backendId. I didn't consider that before, because I was trying
not to get tangled up with BackendIds for various reasons, not least
that that's yet another lock + O(n) search.
It seems likely that getting from vxid to latch will be less clumsy in
the near future. That refactoring and harmonising of backend
identifiers seems like a great idea to me. Here's a version that
anticipates that, using vxid->backendId to wake a sleeping
SERIALIZABLE READ ONLY DEFERRABLE backend, without having to add a new
member to the struct.
A session ID seems a bit heavy just to indicate whether a backend has
exited.
Yeah. A Greenplum-like session ID might eventually be necessary in a
world where sessions are divorced from processes and handled by a pool
of worker threads, though. /me gazes towards the horizon
[1]: /messages/by-id/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
Attachments:
v5-0001-Optimize-ProcSendSignal.patchtext/x-patch; charset=US-ASCII; name=v5-0001-Optimize-ProcSendSignal.patchDownload
From b284d8f29efc1c16c3aa75b64d9a940bcb74872c Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Tue, 3 Aug 2021 10:02:15 +1200
Subject: [PATCH v5 1/2] 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.
In the case of buffer pin waits, we switch to storing the pgprocno of
the waiter. In the case of SERIALIZABLE READ ONLY DEFERRABLE waits, we
derive the pgprocno from the vxid (though that's not yet as efficient as
it could be).
Reviewed-by: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Reviewed-by: Ashwin Agrawal <ashwinstar@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD%3Dq9HmwsfRRb-w%40mail.gmail.com
---
src/backend/access/transam/xlog.c | 3 --
src/backend/storage/buffer/buf_init.c | 3 +-
src/backend/storage/buffer/bufmgr.c | 10 +++---
src/backend/storage/lmgr/predicate.c | 8 ++++-
src/backend/storage/lmgr/proc.c | 49 ++-------------------------
src/include/storage/buf_internals.h | 2 +-
src/include/storage/proc.h | 6 +---
7 files changed, 19 insertions(+), 62 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f84c0bb01e..1574362724 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7308,9 +7308,6 @@ StartupXLOG(void)
/* Also ensure XLogReceiptTime has a sane value */
XLogReceiptTime = GetCurrentTimestamp();
- /* Allow ProcSendSignal() to find us, for buffer pin wakeups. */
- PublishStartupProcessInformation();
-
/*
* Let postmaster know we've started redo now, so that it can launch
* the archiver if necessary.
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 33d99f604a..fb86c8706c 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..fbc0caca5c 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -207,6 +207,7 @@
#include "storage/predicate_internals.h"
#include "storage/proc.h"
#include "storage/procarray.h"
+#include "storage/sinvaladt.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
@@ -3669,7 +3670,12 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
*/
if (SxactIsDeferrableWaiting(roXact) &&
(SxactIsROUnsafe(roXact) || SxactIsROSafe(roXact)))
- ProcSendSignal(roXact->pid);
+ {
+ PGPROC *proc;
+
+ proc = BackendIdGetProc(roXact->vxid.backendId);
+ ProcSendSignal(proc->pgprocno);
+ }
possibleUnsafeConflict = nextConflict;
}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b7d9da0aa9..7f2111c6e0 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/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/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
v5-0002-Remove-PGPROC-s-redundant-pgprocno-member.patchtext/x-patch; charset=US-ASCII; name=v5-0002-Remove-PGPROC-s-redundant-pgprocno-member.patchDownload
From 562657ea3f7be124a6c6b6d1e7450da2431a54a0 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 v5 2/2] Remove PGPROC's redundant pgprocno member.
It's derivable with pointer arithmetic.
Author: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD%3Dq9HmwsfRRb-w%40mail.gmail.com
---
src/backend/access/transam/clog.c | 2 +-
src/backend/access/transam/twophase.c | 3 +--
src/backend/access/transam/xlog.c | 2 +-
src/backend/postmaster/bgwriter.c | 2 +-
src/backend/postmaster/pgarch.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 6 +++---
src/backend/storage/ipc/procarray.c | 6 +++---
src/backend/storage/lmgr/condition_variable.c | 12 ++++++------
src/backend/storage/lmgr/lwlock.c | 6 +++---
src/backend/storage/lmgr/predicate.c | 2 +-
src/backend/storage/lmgr/proc.c | 8 +++-----
src/backend/utils/adt/lockfuncs.c | 1 +
src/include/storage/lock.h | 2 +-
src/include/storage/proc.h | 6 +++++-
14 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 3ea16a270a..d0557f6c55 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -464,7 +464,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
&nextidx,
- (uint32) proc->pgprocno))
+ (uint32) GetPGProcNumber(proc)))
break;
}
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6d3efb49a4..0fe8a59cf0 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -279,7 +279,7 @@ TwoPhaseShmemInit(void)
TwoPhaseState->freeGXacts = &gxacts[i];
/* associate it with a PGPROC assigned by InitProcGlobal */
- gxacts[i].pgprocno = PreparedXactProcs[i].pgprocno;
+ gxacts[i].pgprocno = GetPGProcNumber(&PreparedXactProcs[i]);
/*
* Assign a unique ID for each dummy proc, so that the range of
@@ -456,7 +456,6 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
/* Initialize the PGPROC entry */
MemSet(proc, 0, sizeof(PGPROC));
- proc->pgprocno = gxact->pgprocno;
SHMQueueElemInit(&(proc->links));
proc->waitStatus = PROC_WAIT_STATUS_OK;
/* We set up the gxact's VXID as InvalidBackendId/XID */
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1574362724..576ba061d8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1666,7 +1666,7 @@ WALInsertLockAcquire(void)
static int lockToTry = -1;
if (lockToTry == -1)
- lockToTry = MyProc->pgprocno % NUM_XLOGINSERT_LOCKS;
+ lockToTry = GetPGProcNumber(MyProc) % NUM_XLOGINSERT_LOCKS;
MyLockNo = lockToTry;
/*
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 5584f4bc24..8c812e906d 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -333,7 +333,7 @@ BackgroundWriterMain(void)
if (rc == WL_TIMEOUT && can_hibernate && prev_hibernate)
{
/* Ask for notification at next buffer allocation */
- StrategyNotifyBgWriter(MyProc->pgprocno);
+ StrategyNotifyBgWriter(GetPGProcNumber(MyProc));
/* Sleep ... */
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 74a7d7c4d0..68de7ee3b8 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -196,7 +196,7 @@ PgArchiverMain(void)
* Advertise our pgprocno so that backends can use our latch to wake us up
* while we're sleeping.
*/
- PgArch->pgprocno = MyProc->pgprocno;
+ PgArch->pgprocno = GetPGProcNumber(MyProc);
pgarch_MainLoop();
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index fb86c8706c..ad95e2f938 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -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_pgprocno == MyProc->pgprocno)
+ buf->wait_backend_pgprocno == GetPGProcNumber(MyProc))
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_pgprocno = MyProc->pgprocno;
+ bufHdr->wait_backend_pgprocno = GetPGProcNumber(MyProc);
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_pgprocno == MyProc->pgprocno)
+ bufHdr->wait_backend_pgprocno == GetPGProcNumber(MyProc))
buf_state &= ~BM_PIN_COUNT_WAITER;
UnlockBufHdr(bufHdr, buf_state);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index c7816fcfb3..a2f53a9ce9 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -492,7 +492,7 @@ ProcArrayAdd(PGPROC *proc)
Assert(allProcs[procno].pgxactoff == index);
/* If we have found our right position in the array, break */
- if (arrayP->pgprocnos[index] > proc->pgprocno)
+ if (arrayP->pgprocnos[index] > GetPGProcNumber(proc))
break;
}
@@ -510,7 +510,7 @@ ProcArrayAdd(PGPROC *proc)
&ProcGlobal->statusFlags[index],
movecount * sizeof(*ProcGlobal->statusFlags));
- arrayP->pgprocnos[index] = proc->pgprocno;
+ arrayP->pgprocnos[index] = GetPGProcNumber(proc);
proc->pgxactoff = index;
ProcGlobal->xids[index] = proc->xid;
ProcGlobal->subxidStates[index] = proc->subxidStatus;
@@ -789,7 +789,7 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
if (pg_atomic_compare_exchange_u32(&procglobal->procArrayGroupFirst,
&nextidx,
- (uint32) proc->pgprocno))
+ (uint32) GetPGProcNumber(proc)))
break;
}
diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 80d70c154c..d0728d17bd 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -57,7 +57,7 @@ ConditionVariableInit(ConditionVariable *cv)
void
ConditionVariablePrepareToSleep(ConditionVariable *cv)
{
- int pgprocno = MyProc->pgprocno;
+ int pgprocno = GetPGProcNumber(MyProc);
/*
* If some other sleep is already prepared, cancel it; this is necessary
@@ -181,10 +181,10 @@ ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
* guarantee not to return spuriously, we'll avoid this obvious case.
*/
SpinLockAcquire(&cv->mutex);
- if (!proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
+ if (!proclist_contains(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink))
{
done = true;
- proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
+ proclist_push_tail(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink);
}
SpinLockRelease(&cv->mutex);
@@ -234,8 +234,8 @@ ConditionVariableCancelSleep(void)
return;
SpinLockAcquire(&cv->mutex);
- if (proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
- proclist_delete(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
+ if (proclist_contains(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink))
+ proclist_delete(&cv->wakeup, GetPGProcNumber(MyProc), cvWaitLink);
else
signaled = true;
SpinLockRelease(&cv->mutex);
@@ -285,7 +285,7 @@ ConditionVariableSignal(ConditionVariable *cv)
void
ConditionVariableBroadcast(ConditionVariable *cv)
{
- int pgprocno = MyProc->pgprocno;
+ int pgprocno = GetPGProcNumber(MyProc);
PGPROC *proc = NULL;
bool have_sentinel = false;
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 862097352b..e3cef6700a 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1080,9 +1080,9 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
/* LW_WAIT_UNTIL_FREE waiters are always at the front of the queue */
if (mode == LW_WAIT_UNTIL_FREE)
- proclist_push_head(&lock->waiters, MyProc->pgprocno, lwWaitLink);
+ proclist_push_head(&lock->waiters, GetPGProcNumber(MyProc), lwWaitLink);
else
- proclist_push_tail(&lock->waiters, MyProc->pgprocno, lwWaitLink);
+ proclist_push_tail(&lock->waiters, GetPGProcNumber(MyProc), lwWaitLink);
/* Can release the mutex now */
LWLockWaitListUnlock(lock);
@@ -1122,7 +1122,7 @@ LWLockDequeueSelf(LWLock *lock)
*/
proclist_foreach_modify(iter, &lock->waiters, lwWaitLink)
{
- if (iter.cur == MyProc->pgprocno)
+ if (iter.cur == GetPGProcNumber(MyProc))
{
found = true;
proclist_delete(&lock->waiters, iter.cur, lwWaitLink);
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index fbc0caca5c..508c8bb37f 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3674,7 +3674,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
PGPROC *proc;
proc = BackendIdGetProc(roXact->vxid.backendId);
- ProcSendSignal(proc->pgprocno);
+ ProcSendSignal(GetPGProcNumber(proc));
}
possibleUnsafeConflict = nextConflict;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 7f2111c6e0..1321786857 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -227,7 +227,6 @@ InitProcGlobal(void)
InitSharedLatch(&(procs[i].procLatch));
LWLockInitialize(&(procs[i].fpInfoLock), LWTRANCHE_LOCK_FASTPATH);
}
- procs[i].pgprocno = i;
/*
* Newly created PGPROCs for normal backends, autovacuum and bgworkers
@@ -1947,10 +1946,9 @@ BecomeLockGroupMember(PGPROC *leader, int pid)
/*
* Get lock protecting the group fields. Note LockHashPartitionLockByProc
- * accesses leader->pgprocno in a PGPROC that might be free. This is safe
- * because all PGPROCs' pgprocno fields are set during shared memory
- * initialization and never change thereafter; so we will acquire the
- * correct lock even if the leader PGPROC is in process of being recycled.
+ * takes a PGPROC that might be free, but it uses only its address. We
+ * will acquire the correct lock even if the leader PGPROC is in the
+ * process of being recycled.
*/
leader_lwlock = LockHashPartitionLockByProc(leader);
LWLockAcquire(leader_lwlock, LW_EXCLUSIVE);
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/lock.h b/src/include/storage/lock.h
index 9b2a421c32..c8e1b0ce1a 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -531,7 +531,7 @@ typedef enum
* used for a given lock group is determined by the group leader's pgprocno.
*/
#define LockHashPartitionLockByProc(leader_pgproc) \
- LockHashPartitionLock((leader_pgproc)->pgprocno)
+ LockHashPartitionLock(GetPGProcNumber(leader_pgproc))
/*
* function prototypes
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 7c1e2efe30..435876772b 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -147,7 +147,6 @@ struct PGPROC
int pgxactoff; /* offset into various ProcGlobal->arrays with
* data mirrored from this PGPROC */
- int pgprocno;
/* These fields are zero while a backend is still starting up: */
BackendId backendId; /* This backend's backend ID (if assigned) */
@@ -311,6 +310,9 @@ extern PGDLLIMPORT PGPROC *MyProc;
* When entering a PGPROC for 2PC transactions with ProcArrayAdd(), the data
* in the dense arrays is initialized from the PGPROC while it already holds
* ProcArrayLock.
+ *
+ * Shared memory data structures can refer to PGPROCs by index in allProcs.
+ * See GetPGProcNumber() and GetPGProcByNumber().
*/
typedef struct PROC_HDR
{
@@ -362,6 +364,8 @@ extern PGPROC *PreparedXactProcs;
/* Accessor for PGPROC given a pgprocno. */
#define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)])
+/* Accessor for pgprocno given a pointer to PGPROC. */
+#define GetPGProcNumber(proc) ((proc) - ProcGlobal->allProcs)
/*
* We set aside some extra PGPROC structures for auxiliary processes,
--
2.30.2
Hey Thomas,
On Mon, Aug 2, 2021 at 6:45 PM Thomas Munro <thomas.munro@gmail.com> wrote:
Hi Soumyadeep,
On Sat, Jul 24, 2021 at 5:26 PM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:On Tue, Jul 20, 2021 at 10:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
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...I tried this out. See attached v4 of your patch with these changes.
I like it. I've moved these changes to a separate patch, 0002, for
separate commit. I tweaked a couple of comments (it's not a position
in the "procarray", well it's a position stored in the procarray, but
that's confusing; I also found a stray comment about leader->pgprocno
that is obsoleted by this change). Does anyone have objections to
this?
Awesome, thanks! Looks good.
I was going to commit the earlier change this morning, but then I read [1].
New idea. Instead of adding pgprocno to SERIALIZABLEXACT (which we
should really be trying to shrink, not grow), let's look it up by
vxid->backendId. I didn't consider that before, because I was trying
not to get tangled up with BackendIds for various reasons, not least
that that's yet another lock + O(n) search.It seems likely that getting from vxid to latch will be less clumsy in
the near future. That refactoring and harmonising of backend
identifiers seems like a great idea to me. Here's a version that
anticipates that, using vxid->backendId to wake a sleeping
SERIALIZABLE READ ONLY DEFERRABLE backend, without having to add a new
member to the struct.
Neat! A Vxid -> PGPROC lookup eventually becomes an O(1) operation with the
changes proposed at the ending paragraph of [1]/messages/by-id/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de.
[1]: /messages/by-id/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
Regards,
Soumyadeep (VMware)
Hi,
On 2021-08-03 13:44:58 +1200, Thomas Munro wrote:
New idea. Instead of adding pgprocno to SERIALIZABLEXACT (which we
should really be trying to shrink, not grow), let's look it up by
vxid->backendId. I didn't consider that before, because I was trying
not to get tangled up with BackendIds for various reasons, not least
that that's yet another lock + O(n) search.It seems likely that getting from vxid to latch will be less clumsy in
the near future.
So this change only makes sense of vxids would start to use pgprocno instead
of backendid, basically?
From b284d8f29efc1c16c3aa75b64d9a940bcb74872c Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Tue, 3 Aug 2021 10:02:15 +1200
Subject: [PATCH v5 1/2] 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.In the case of buffer pin waits, we switch to storing the pgprocno of
the waiter. In the case of SERIALIZABLE READ ONLY DEFERRABLE waits, we
derive the pgprocno from the vxid (though that's not yet as efficient as
it could be).
That's kind of an understatement :)
-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); }
I think some validation of the pgprocno here would be a good idea. I'm worried
that something could cause e.g. INVALID_PGPROCNO to be passed in, and suddenly
we're modifying random memory. That could end up being a pretty hard bug to
catch, because we might not even notice that the right latch isn't set...
From 562657ea3f7be124a6c6b6d1e7450da2431a54a0 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 v5 2/2] Remove PGPROC's redundant pgprocno member.It's derivable with pointer arithmetic.
Author: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Discussion:
/messages/by-id/CA+hUKGLYRyDaneEwz5Uya_OgFLMx5BgJfkQSD=q9HmwsfRRb-w@mail.gmail.com
/* Accessor for PGPROC given a pgprocno. */ #define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)]) +/* Accessor for pgprocno given a pointer to PGPROC. */ +#define GetPGProcNumber(proc) ((proc) - ProcGlobal->allProcs)
I'm not sure this is a good idea. There's no really cheap way for the compiler
to compute this, because sizeof(PGPROC) isn't a power of two. Given that
PGPROC is ~880 bytes, I don't see all that much gain in getting rid of
->pgprocno.
Yes, compilers can optimize away the super expensive division, but it'll end
up as something like subtraction, shift, multiplication - not that cheap
either. And I suspect it'll often have to first load the ProcGlobal via the
GOT as well...
Greetings,
Andres Freund
On Tue, Aug 3, 2021 at 2:56 PM Andres Freund <andres@anarazel.de> wrote:
On 2021-08-03 13:44:58 +1200, Thomas Munro wrote:
In the case of buffer pin waits, we switch to storing the pgprocno of
the waiter. In the case of SERIALIZABLE READ ONLY DEFERRABLE waits, we
derive the pgprocno from the vxid (though that's not yet as efficient as
it could be).That's kind of an understatement :)
I abandoned the vxid part for now and went back to v3. If/when
BackendId is replaced with or becomes synonymous with pgprocno, we can
make this change and drop the pgprocno member from SERIALIZABLEXACT.
+ SetLatch(&ProcGlobal->allProcs[pgprocno].procLatch);
I think some validation of the pgprocno here would be a good idea. I'm worried
that something could cause e.g. INVALID_PGPROCNO to be passed in, and suddenly
we're modifying random memory. That could end up being a pretty hard bug to
catch, because we might not even notice that the right latch isn't set...
Added.
/* Accessor for PGPROC given a pgprocno. */ #define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)]) +/* Accessor for pgprocno given a pointer to PGPROC. */ +#define GetPGProcNumber(proc) ((proc) - ProcGlobal->allProcs)I'm not sure this is a good idea. There's no really cheap way for the compiler
to compute this, because sizeof(PGPROC) isn't a power of two. Given that
PGPROC is ~880 bytes, I don't see all that much gain in getting rid of
->pgprocno.
Yeah, that would need some examination; 0002 patch abandoned for now.
Pushed.