8000 Centralize decision-making about where to get a backend's PGPROC. · postgres/postgres@6f2871f · GitHub
[go: up one dir, main page]
Skip to content

Commit 6f2871f

Browse files
robertmhaasrobertmhaas
committed
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 e280c63.
1 parent 95f4e59 commit 6f2871f

File tree

2 files changed

+27
-29
lines changed

2 files changed

+27
-29
lines changed

‎src/backend/storage/lmgr/proc.c

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -242,18 +242,21 @@ InitProcGlobal(void)
242242
/* PGPROC for normal backend, add to freeProcs list */
243243
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->freeProcs;
244244
ProcGlobal->freeProcs = &procs[i];
245+
procs[i].procgloballist = &ProcGlobal->freeProcs;
245246
}
246247
else if (i < MaxConnections + autovacuum_max_workers + 1)
247248
{
248249
/* PGPROC for AV launcher/worker, add to autovacFreeProcs list */
249250
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
250251
ProcGlobal->autovacFreeProcs = &procs[i];
252+
procs[i].procgloballist = &ProcGlobal->autovacFreeProcs;
251253
}
252254
else if (i < MaxBackends)
253255
{
254256
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
255257
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
256258
ProcGlobal->bgworkerFreeProcs = &procs[i];
259+
procs[i].procgloballist = &ProcGlobal->bgworkerFreeProcs;
257260
}
258261

259262
/* Initialize myProcLocks[] shared memory queues. */
@@ -281,6 +284,7 @@ InitProcess(void)
281284
{
282285
/* use volatile pointer to prevent code rearrangement */
283286
volatile PROC_HDR *procglobal = ProcGlobal;
287+
PGPROC * volatile * procgloballist;
284288

285289
/*
286290
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -292,9 +296,17 @@ InitProcess(void)
292296
if (MyProc != NULL)
293297
elog(ERROR, "you already exist");
294298

299+
/* Decide which list should supply our PGPROC. */
300+
if (IsAnyAutoVacuumProcess())
301+
procgloballist = &procglobal->autovacFreeProcs;
302+
else if (IsBackgroundWorker)
303+
procgloballist = &procglobal->bgworkerFreeProcs;
304+
else
305+
procgloballist = &procglobal->freeProcs;
306+
295307
/*
296-
* Try to get a proc struct from the free list. If this fails, we must be
297-
* out of PGPROC structures (not to mention semaphores).
308+
* Try to get a proc struct from the appropriate free list. If this
309+
* fails, we must be out of PGPROC structures (not to mention semaphores).
298310
*
299311
* While we are holding the ProcStructLock, also copy the current shared
300312
* estimate of spins_per_delay to local storage.
@@ -303,21 +315,11 @@ InitProcess(void)
303315

304316
set_spins_per_delay(procglobal->spins_per_delay);
305317

306-
if (IsAnyAutoVacuumProcess())
307-
MyProc = procglobal->autovacFreeProcs;
308-
else if (IsBackgroundWorker)
309-
MyProc = procglobal->bgworkerFreeProcs;
310-
else
311-
MyProc = procglobal->freeProcs;
318+
MyProc = *procgloballist;
312319

313320
if (MyProc != NULL)
314321
{
315-
if (IsAnyAutoVacuumProcess())
316-
procglobal->autovacFreeProcs = (PGPROC *) MyProc->links.next;
317-
else if (IsBackgroundWorker)
318-
procglobal->bgworkerFreeProcs = (PGPROC *) MyProc->links.next;
319-
else
320-
procglobal->freeProcs = (PGPROC *) MyProc->links.next;
322+
*procgloballist = (PGPROC *) MyProc->links.next;
321323
SpinLockRelease(ProcStructLock);
322324
}
323325
else
@@ -335,6 +337,12 @@ InitProcess(void)
335337
}
336338
MyPgXact = &ProcGlobal->allPgXact[MyProc->pgprocno];
337339

340+
/*
341+
* Cross-check that the PGPROC is of the type we expect; if this were
342+
* not the case, it would get returned to the wrong list.
343+
*/
344+
Assert(MyProc->procgloballist == procgloballist);
345+
338346
/*
339347
* Now that we have a PGPROC, mark ourselves as an active postmaster
340348
* child; this is so that the postmaster can detect it if we exit without
@@ -761,6 +769,7 @@ ProcKill(int code, Datum arg)
761769
/* use volatile pointer to prevent code rearrangement */
762770
volatile PROC_HDR *procglobal = ProcGlobal;
763771
PGPROC *proc;
772+
PGPROC * volatile * procgloballist;
764773

765774
Assert(MyProc != NULL);
766775

@@ -799,24 +808,12 @@ ProcKill(int code, Datum arg)
799808
MyProc = NULL;
800809
DisownLatch(&proc->procLatch);
801810

811+
procgloballist = proc->procgloballist;
802812
SpinLockAcquire(ProcStructLock);
803813

804814
/* Return PGPROC structure (and semaphore) to appropriate freelist */
805-
if (IsAnyAutoVacuumProcess())
806-
{
807-
proc->links.next = (SHM_QUEUE *) procglobal->autovacFreeProcs;
808-
procglobal->autovacFreeProcs = proc;
809-
}
810-
else if (IsBackgroundWorker)
811-
{
812-
proc->links.next = (SHM_QUEUE *) procglobal->bgworkerFreeProcs;
813-
procglobal->bgworkerFreeProcs = proc;
814-
}
815-
else
816-
{
817-
proc->links.next = (SHM_QUEUE *) procglobal->freeProcs;
818-
procglobal->freeProcs = proc;
819-
}
815+
proc->links.next = (SHM_QUEUE *) *procgloballist;
816+
*procgloballist = proc;
820817

821818
/* Update shared estimate of spins_per_delay */
822819
procglobal->spins_per_delay = update_spins_per_delay(procglobal->spins_per_delay);

‎src/include/storage/proc.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ struct PGPROC
7878
{
7979
/* proc->links MUST BE FIRST IN STRUCT (see ProcSleep,ProcWakeup,etc) */
8080
SHM_QUEUE links; /* list link if process is in a list */
81+
PGPROC **procgloballist; /* procglobal list that owns this PGPROC */
8182

8283
PGSemaphoreData sem; /* ONE semaphore to sleep on */
8384
int waitStatus; /* STATUS_WAITING, STATUS_OK or STATUS_ERROR */

0 commit comments

Comments
 (0)
0