On Mon, Jun 27, 2016 at 10:21 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Sun, Jun 26, 2016 at 3:57 PM, Julien Rouhaud > <julien.rouh...@dalibo.com> wrote: >> On 26/06/2016 08:37, Amit Kapila wrote: >>> >>> @@ -370,6 +379,8 @@ ForgetBackgroundWorker(slist_mutable_iter *cur) >>> Assert(rw->rw_shmem_slot < >>> max_worker_processes); >>> slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot]; >>> slot->in_use = >>> false; >>> + if (slot->parallel) >>> + BackgroundWorkerData->parallel_terminate_count++; >>> >>> I think operations on parallel_terminate_count are not safe. >>> ForgetBackgroundWorker() and RegisterDynamicBackgroundWorker() can try >>> to read write at same time. It seems you need to use atomic >>> operations to ensure safety. >>> >>> >> >> But operations on parallel_terminate_count are done by the postmaster, >> and per Robert's previous email postmaster can't use atomics: >> > > I think as the parallel_terminate_count is only modified by postmaster > and read by other processes, such an operation will be considered > atomic. We don't need to specifically make it atomic unless multiple > processes are allowed to modify such a counter. Although, I think we > need to use some barriers in code to make it safe. > > 1. > @@ -272,6 +279,8 @@ BackgroundWorkerStateChange(void) > pg_memory_barrier(); > > slot->pid = 0; > slot->in_use = false; > + if (slot->parallel) > + > BackgroundWorkerData->parallel_terminate_count++; > > I think the check of slot->parallel and increment to > parallel_terminate_count should be done before marking slot->in_use to > false. Consider the case if slot->in_use is marked as false and then > before next line execution, one of the backends registers dynamic > worker (non-parallel worker), so that > backend can see this slot as free and use it. It will also mark the > parallel flag of slot as false. Now when postmaster will check the > slot->parallel flag, it will find it false and then skip the increment > to parallel_terminate_count. >
If you agree with above theory, then you need to use write barrier as well after incrementing the parallel_terminate_count to avoid re-ordering with slot->in_use flag. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
