Re: [HACKERS] parallel.c oblivion of worker-startup failures

On Wed, Dec 13, 2017 at 1:41 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I have tried to reproduce this situation by adding error case in
> worker (just before worker returns success ('X' message)) and by
> having breakpoint in WaitForParallelWorkersToFinish. What, I noticed
> is that worker is not allowed to exit till it receives some ack from
> master (basically worker waits at SendProcSignal after sending
> message) and error is reported properly.

SendProcSignal doesn't look to me like it can do anything that can
block, so I don't understand this.

> So, if I understand correctly, this means that it will return an error
> even if there is a problem in one worker (exited or not started) even
> though other workers would have easily finished the work. Won't such
> an error can lead to situations where after running the query for one
> hour the user might see some failure just because one of the many
> workers is not started (or some similar failure) even though the query
> would have been completed without that? If so, that doesn't sound
> like a sensible behavior.

I think it's the *only* sensible behavior. parallel.c does not know
that the query would have completed successfully without that worker,
or at least it doesn't know that it would have returned the right
answer. It *might* be the case that the caller wasn't depending on
that worker to do anything, but parallel.c doesn't know that.

> This also doesn't appear to be completely safe. If we add
> proc_exit(1) after attaching to error queue (say after
> pq_set_parallel_master) in the worker, then it will lead to *hang* as
> anyone_alive will still be false and as it will find that the sender
> is set for the error queue, it won't return any failure. Now, I think
> even if we check worker status (which will be stopped) and break after
> the new error condition, it won't work as it will still return zero
> rows in the case reported by you above.

Hmm, there might still be a problem there. I was thinking that once
the leader attaches to the queue, we can rely on the leader reaching
"ERROR: lost connection to parallel worker" in HandleParallelMessages.
However, that might not work because nothing sets
ParallelMessagePending in that case. The worker will have detached
the queue via shm_mq_detach_callback, but the leader will only
discover the detach if it actually tries to read from that queue.

> I think if before making an assumption not to scan locally if we have
> a check to see whether workers are started, then my patch should work
> fine and we don't need to add any additional checks. Also, it won't
> create any performance issue as we will perform such a check only when
> force_parallel_mode is not off or parallel leader participation is off
> which I think are not common cases.

My instinct is that we're going to have a lot of subtle bugs if
clients of parallel.c can't count on nworkers_launched to be accurate.
If clients didn't need that value, we could just have nworkers and let
the callers figure it out, but nobody wants that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company