Hi,
While reading code for another work, I spotted something suspicious that might be an oversight in “[f1e251be8] Allow
bgworkersto be terminated for database-related commands”.
In TerminateBackgroundWorkersForDatabase(Oid databaseId)
```
/*
* Iterate through slots, looking for workers connected to the given
* database.
*/
for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; slotno++)
{
BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];
if (slot->in_use &&
(slot->worker.bgw_flags & BGWORKER_INTERRUPTIBLE))
{
PGPROC *proc = BackendPidGetProc(slot->pid);
if (proc && proc->databaseId == databaseId)
{
slot->terminate = true;
signal_postmaster = true;
elog(DEBUG1, "termination requested for worker (PID %d) on database %u",
(int) slot->pid, databaseId);
}
}
}
```
BackendPidGetProc() returns a PGPROC pointer after releasing ProcArrayLock, so the returned PGPROC is no longer
protected.The corresponding entry in allProcs could be recycled after the function returns, in which case proc could
referto a different process when proc->databaseId is accessed.
Similarly, the postmaster may update slot->pid between the lookup and the elog(), so the logged PID has a small race as
well.
I don’t have a concrete repro for this, but I think we should hold ProcArrayLock and use BackendPidGetProcWithLock()
instead.See the attached patch for the changes.
One thing to point out is that, to keep the lock section as small as possible, I added a local variable, terminate, so
thatelog() can be placed outside the lock section. Maybe I’m overly cautious here, please let me know if that is
unnecessary.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
