On 27/06/2016 07:18, Amit Kapila wrote: > On Mon, Jun 27, 2016 at 10:21 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> >> 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. >
I totally agree, I didn't thought about this corner case. There's already a pg_memory_barrier() call in BackgroundWorkerStateChange(), to avoid reordering the notify_pid load. Couldn't we use it to also make sure the parallel_terminate_count increment happens before the slot->in_use store? I guess that a write barrier will be needed in ForgetBacgroundWorker(). >> 2. >> + if (parallel && (BackgroundWorkerData->parallel_register_count - >> + >> BackgroundWorkerData->parallel_terminate_count) >= >> + >> max_parallel_workers) >> + { >> + LWLockRelease(BackgroundWorkerLock); >> + return >> false; >> + } >>+ >> >> I think we need a read barrier here, so that this check doesn't get >> reordered with the for loop below it. You mean between the end of this block and the for loop? (Sorry, I'm not at all familiar with the pg_barrier mechanism). >> Also, see if you find the code >> more readable by moving the after && part of check to next line. I think I'll just pgindent the file. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers