Re: Fix race in background worker termination for database

On Fri, Jul 24, 2026 at 11:31 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:

> Hi,
>
> While reading code for another work, I spotted something suspicious that
> might be an oversight in “[f1e251be8] Allow bgworkers to 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 refer to 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 that elog() 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/
>
>
> The fix looks correct to me. BackendPidGetProc() releases ProcArrayLock
before returning, so the returned PGPROC may be removed from the proc array
and reused before its databaseId is examined. Keeping ProcArrayLock held
across both BackendPidGetProcWithLock() and the databaseId check closes
that race.

I suggest a small changes:

Copy slot->pid before acquiring ProcArrayLock, since ProcArrayLock does not

protect the background worker slot and there is no reason to include that
read

in the ProcArrayLock critical section.

Regards,
Haibo