From 6f2871f12e9fba5deec4296cfe12e85c140261c4 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Tue, 28 Jul 2015 14:51:57 -0400
Subject: [PATCH] Centralize decision-making about where to get a backend's
PGPROC.
This code was originally written as part of parallel query effort, but
it seems to have independent value, because if we make one decision
about where to get a PGPROC when we allocate and then put it back on a
different list at backend-exit time, bad things happen. This isn't
just a theoretical risk; we fixed an actual problem of this type in
commit e280c630a87e1b8325770c6073097d109d79a00f.
---
src/backend/storage/lmgr/proc.c | 55 ++++++++++++++++-----------------
src/include/storage/proc.h | 1 +
2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 455ad266340..324347b2c58 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -242,18 +242,21 @@ InitProcGlobal(void)
/* PGPROC for normal backend, add to freeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->freeProcs;
ProcGlobal->freeProcs = &procs[i];
+ procs[i].procgloballist = &ProcGlobal->freeProcs;
}
else if (i < MaxConnections + autovacuum_max_workers + 1)
{
/* PGPROC for AV launcher/worker, add to autovacFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
ProcGlobal->autovacFreeProcs = &procs[i];
+ procs[i].procgloballist = &ProcGlobal->autovacFreeProcs;
}
else if (i < MaxBackends)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
ProcGlobal->bgworkerFreeProcs = &procs[i];
+ procs[i].procgloballist = &ProcGlobal->bgworkerFreeProcs;
}
/* Initialize myProcLocks[] shared memory queues. */
@@ -281,6 +284,7 @@ InitProcess(void)
{
/* use volatile pointer to prevent code rearrangement */
volatile PROC_HDR *procglobal = ProcGlobal;
+ PGPROC * volatile * procgloballist;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -292,9 +296,17 @@ InitProcess(void)
if (MyProc != NULL)
elog(ERROR, "you already exist");
+ /* Decide which list should supply our PGPROC. */
+ if (IsAnyAutoVacuumProcess())
+ procgloballist = &procglobal->autovacFreeProcs;
+ else if (IsBackgroundWorker)
+ procgloballist = &procglobal->bgworkerFreeProcs;
+ else
+ procgloballist = &procglobal->freeProcs;
+
/*
- * Try to get a proc struct from the free list. If this fails, we must be
- * out of PGPROC structures (not to mention semaphores).
+ * Try to get a proc struct from the appropriate free list. If this
+ * fails, we must be out of PGPROC structures (not to mention semaphores).
*
* While we are holding the ProcStructLock, also copy the current shared
* estimate of spins_per_delay to local storage.
@@ -303,21 +315,11 @@ InitProcess(void)
set_spins_per_delay(procglobal->spins_per_delay);
- if (IsAnyAutoVacuumProcess())
- MyProc = procglobal->autovacFreeProcs;
- else if (IsBackgroundWorker)
- MyProc = procglobal->bgworkerFreeProcs;
- else
- MyProc = procglobal->freeProcs;
+ MyProc = *procgloballist;
if (MyProc != NULL)
{
- if (IsAnyAutoVacuumProcess())
- procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
- else if (IsBackgroundWorker)
- procglobal->bgworkerFreeProcs = (PGPROC *) MyProc->links.next;
- else
- procglobal->freeProcs = (PGPROC *) MyProc->links.next;
+ *procgloballist = (PGPROC *) MyProc->links.next;
SpinLockRelease(ProcStructLock);
}
else
@@ -335,6 +337,12 @@ InitProcess(void)
}
MyPgXact = &ProcGlobal->allPgXact[MyProc->pgprocno];
+ /*
+ * Cross-check that the PGPROC is of the type we expect; if this were
+ * not the case, it would get returned to the wrong list.
+ */
+ Assert(MyProc->procgloballist == procgloballist);
+
/*
* Now that we have a PGPROC, mark ourselves as an active postmaster
* child; this is so that the postmaster can detect it if we exit without
@@ -761,6 +769,7 @@ ProcKill(int code, Datum arg)
/* use volatile pointer to prevent code rearrangement */
volatile PROC_HDR *procglobal = ProcGlobal;
PGPROC *proc;
+ PGPROC * volatile * procgloballist;
Assert(MyProc != NULL);
@@ -799,24 +808,12 @@ ProcKill(int code, Datum arg)
MyProc = NULL;
DisownLatch(&proc->procLatch);
+ procgloballist = proc->procgloballist;
SpinLockAcquire(ProcStructLock);
/* Return PGPROC structure (and semaphore) to appropriate freelist */
- if (IsAnyAutoVacuumProcess())
- {
- proc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
- procglobal->autovacFreeProcs = proc;
- }
- else if (IsBackgroundWorker)
- {
- proc->links.next = (SHM_QUEUE *) procglobal->bgworkerFreeProcs;
- procglobal->bgworkerFreeProcs = proc;
- }
- else
- {
- proc->links.next = (SHM_QUEUE *) procglobal->freeProcs;
- procglobal->freeProcs = proc;
- }
+ proc->links.next = (SHM_QUEUE *) *procgloballist;
+ *procgloballist = proc;
/* Update shared estimate of spins_per_delay */
procglobal->spins_per_delay = update_spins_per_delay(procglobal->spins_per_delay);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e807a2e020d..202a672bca5 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -78,6 +78,7 @@ struct PGPROC
{
/* proc->links MUST BE FIRST IN STRUCT (see ProcSleep,ProcWakeup,etc) */
SHM_QUEUE links; /* list link if process is in a list */
+ PGPROC **procgloballist; /* procglobal list that owns this PGPROC */
PGSemaphoreData sem; /* ONE semaphore to sleep on */
int waitStatus; /* STATUS_WAITING, STATUS_OK or STATUS_ERROR */
--
GitLab