On Wed, Jan 24, 2018 at 12:13 PM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: > On Thu, Jan 25, 2018 at 8:54 AM, Peter Geoghegan <p...@bowt.ie> wrote: >> I have used Thomas' chaos-monkey-fork-process.patch to verify: >> >> 1. The problem of fork failure causing nbtsort.c to wait forever is a >> real problem. Sure enough, the coding pattern within >> _bt_leader_heapscan() can cause us to wait forever even with commit >> 2badb5afb89cd569500ef7c3b23c7a9d11718f2f, more or less as a >> consequence of the patch not using tuple queues (it uses the new >> tuplesort sharing thing instead). > > Just curious: does the attached also help?
I can still reproduce the problem without the fix I described (which does work), using your patch instead. Offhand, I suspect that the way you set ParallelMessagePending may not always leave it set when it should be. >> 2. Simply adding a single call to WaitForParallelWorkersToFinish() >> within _bt_leader_heapscan() before waiting on our condition variable >> fixes the problem -- errors are reliably propagated, and we never end >> up waiting forever. > > That does seem like a nice, simple solution and I am not against it. > The niggling thing that bothers me about it, though, is that it > requires the client of parallel.c to follow a slightly complicated > protocol or risk a rare obscure failure mode, and recognise the cases > where that's necessary. Specifically, if you're not blocking in a > shm_mq wait loop, then you must make a call to this new interface > before you do any other kind of latch wait, but if you get that wrong > you'll probably not notice since fork failure is rare! It seems like > it'd be nicer if we could figure out a way to make it so that any > latch/CFI loop would automatically be safe against fork failure. It would certainly be nicer, but I don't see much risk if we add a comment next to nworkers_launched that said: Don't trust this until you've called (Amit's proposed) WaitForParallelWorkersToAttach() function, unless you're using the tuple queue infrastructure, which lets you not need to directly care about the distinction between a launched worker never starting, and a launched worker successfully completing. While I agree with what Robert said on the other thread -- "I guess that works, but it seems more like blind luck than good design. Parallel CREATE INDEX fails to be as "lucky" as Gather" -- that doesn't mean that that situation cannot be formalized. And even if it isn't formalized, then I think that that will probably be because Gather ends up doing almost the same thing. -- Peter Geoghegan
