Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 6bcb519

Browse files
committed
Be more careful about barriers when releasing BackgroundWorkerSlots.
ForgetBackgroundWorker lacked any memory barrier at all, while BackgroundWorkerStateChange had one but unaccountably did additional manipulation of the slot after the barrier. AFAICS, the rule must be that the barrier is immediately before setting or clearing slot->in_use. It looks like back in 9.6 when ForgetBackgroundWorker was first written, there might have been some case for not needing a barrier there, but I'm not very convinced of that --- the fact that the load of bgw_notify_pid is in the caller doesn't seem to guarantee no memory ordering problem. So patch 9.6 too. It's likely that this doesn't fix any observable bug on Intel hardware, but machines with weaker memory ordering rules could have problems here. Discussion: https://postgr.es/m/4046084.1620244003@sss.pgh.pa.us
1 parent 0a53600 commit 6bcb519

File tree

1 file changed

+12
-1
lines changed

1 file changed

+12
-1
lines changed

src/backend/postmaster/bgworker.c

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,11 @@ BackgroundWorkerStateChange(bool allow_new_workers)
328328
notify_pid = slot->worker.bgw_notify_pid;
329329
if ((slot->worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0)
330330
BackgroundWorkerData->parallel_terminate_count++;
331-
pg_memory_barrier();
332331
slot->pid = 0;
332+
333+
pg_memory_barrier();
333334
slot->in_use = false;
335+
334336
if (notify_pid != 0)
335337
kill(notify_pid, SIGUSR1);
336338

@@ -417,6 +419,8 @@ BackgroundWorkerStateChange(bool allow_new_workers)
417419
* points to it. This convention allows deletion of workers during
418420
* searches of the worker list, and saves having to search the list again.
419421
*
422+
* Caller is responsible for notifying bgw_notify_pid, if appropriate.
423+
*
420424
* This function must be invoked only in the postmaster.
421425
*/
422426
void
@@ -429,9 +433,16 @@ ForgetBackgroundWorker(slist_mutable_iter *cur)
429433

430434
Assert(rw->rw_shmem_slot < max_worker_processes);
431435
slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
436+
Assert(slot->in_use);
437+
438+
/*
439+
* We need a memory barrier here to make sure that the update of
440+
* parallel_terminate_count completes before the store to in_use.
441+
*/
432442
if ((rw->rw_worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0)
433443
BackgroundWorkerData->parallel_terminate_count++;
434444

445+
pg_memory_barrier();
435446
slot->in_use = false;
436447

437448
ereport(DEBUG1,

0 commit comments

Comments
 (0)