diff options
| author | Tom Lane <tgl@sss.pgh.pa.us> | 2021-05-15 12:21:06 -0400 |
|---|---|---|
| committer | Tom Lane <tgl@sss.pgh.pa.us> | 2021-05-15 12:21:06 -0400 |
| commit | 6fcbaea7af4323e7e2847fb75fde313c832f4947 (patch) | |
| tree | 868aaefa87a0c5314d8d40d42523d756df527bbc /src | |
| parent | 037cc13f413de8d1d7e332e441d5d71451f2d336 (diff) | |
| download | postgresql-6fcbaea7af4323e7e2847fb75fde313c832f4947.tar.gz postgresql-6fcbaea7af4323e7e2847fb75fde313c832f4947.zip | |
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
Diffstat (limited to 'src')
| -rw-r--r-- | src/backend/postmaster/bgworker.c | 13 |
1 files changed, 12 insertions, 1 deletions
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 80e9c522482..cf4c0d636c5 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -328,9 +328,11 @@ BackgroundWorkerStateChange(bool allow_new_workers) notify_pid = slot->worker.bgw_notify_pid; if ((slot->worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0) BackgroundWorkerData->parallel_terminate_count++; - pg_memory_barrier(); slot->pid = 0; + + pg_memory_barrier(); slot->in_use = false; + if (notify_pid != 0) kill(notify_pid, SIGUSR1); @@ -417,6 +419,8 @@ BackgroundWorkerStateChange(bool allow_new_workers) * points to it. This convention allows deletion of workers during * searches of the worker list, and saves having to search the list again. * + * Caller is responsible for notifying bgw_notify_pid, if appropriate. + * * This function must be invoked only in the postmaster. */ void @@ -429,9 +433,16 @@ ForgetBackgroundWorker(slist_mutable_iter *cur) Assert(rw->rw_shmem_slot < max_worker_processes); slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot]; + Assert(slot->in_use); + + /* + * We need a memory barrier here to make sure that the update of + * parallel_terminate_count completes before the store to in_use. + */ if ((rw->rw_worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0) BackgroundWorkerData->parallel_terminate_count++; + pg_memory_barrier(); slot->in_use = false; ereport(DEBUG1, |