On Mon, Jan 29, 2018 at 4:01 AM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Sat, Jan 27, 2018 at 12:14 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> I also found that all of these errors were propagated. The patch helps
> parallel CREATE INDEX as expected/designed.
>

Great!

> Some small things that I noticed about the patch:
>
> * Maybe "if (!known_started_workers[i])" should be converted to "if
> (known_started_workers[i]) continue;", to decrease the indentation
> level of the WaitForParallelWorkersToAttach() loop.
>

Changed as per suggestion.

> * There might be some opportunity to share some of the new code with
> the code recently committed to WaitForParallelWorkersToFinish(). For
> one thing, the logic in this block could be refactored into a
> dedicated function that is called by both
> WaitForParallelWorkersToAttach() and WaitForParallelWorkersToFinish():
>
>> +               else if (status == BGWH_STOPPED)
>> +               {
>> +                   /*
>> +                    * Check whether the worker ended up stopped without ever
>> +                    * attaching to the error queue.  If so, the postmaster
>> +                    * was unable to fork the worker or it exited without
>> +                    * initializing properly.  We must throw an error, since
>> +                    * the caller may have been expecting the worker to do
>> +                    * some work before exiting.
>> +                    */
>> +                   mq = shm_mq_get_queue(pcxt->worker[i].error_mqh);
>> +                   if (shm_mq_get_sender(mq) == NULL)
>> +                       ereport(ERROR,
>> +                               
>> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +                                errmsg("parallel worker failed to 
>> initialize"),
>> +                                errhint("More details may be available in 
>> the server log.")));
>> +
>> +                   known_started_workers[i] = true;
>> +                   ++nknown_started_workers;
>> +               }
>

I had thought about this earlier but left it as the common code was
too less, however as you have pointed out, I had extracted the common
code into a separate function.

> * If we don't actually commit the patch to make nodeGather.c call
> WaitForParallelWorkersToAttach(), which I suspect will happen, then I
> think we should instead at least have a comment saying why it's okay
> that we don't call WaitForParallelWorkersToAttach(). If we go this
> way, the comment should directly replace the
> modify_gather_to_wait_for_attach_v1.patch call to
> WaitForParallelWorkersToAttach() -- this comment should go in
> ExecGather().
>
> * Maybe the comments at the top of WaitForParallelWorkersToAttach()
> should at least allude to the ExecGather() special case I just
> mentioned.
>

I think we should not touch anything related to Gather (merge) as they
don't need it for the purpose of correctness.  However, we might want
to improve them by using this new API at a certain point if the need
arises.  I guess we can use this API to detect failures early.

> * Maybe the comments at the top of WaitForParallelWorkersToAttach()
> should also advise callers that it's a good idea to try to do other
> leader-only work that doesn't involve a WaitLatch() before they call.
>
> In general, WaitForParallelWorkersToAttach() needs to be called before
> any WaitLatch() (or barrier wait, or condition variable wait) that
> waits on workers, and after workers are first launched, but callers
> may be able to arrange to do plenty of other work, just like nbtsort.c
> does. To be clear: IMHO calling WaitForParallelWorkersToAttach()
> should be the rule, not the exception.
>
> * Finally, perhaps the comments at the top of
> WaitForParallelWorkersToAttach() should also describe how it relates
> to WaitForParallelWorkersToFinish().
>
> ISTM that WaitForParallelWorkersToAttach() is a subset of
> WaitForParallelWorkersToFinish(), that does all that is needed to rely
> on nworkers_launched actually being the number of worker processes
> that are attached to the error queue. As such, caller can expect
> propagation of errors from workers using standard parallel message
> interrupts once WaitForParallelWorkersToAttach() returns. You probably
> shouldn't directly reference nworkers_launched, though, since that
> doesn't necessarily have to be involved for parallel client code to
> run into trouble. (You only need to wait on workers changing something
> in shared memory, and failing to actively inform leader of failure
> through a parallel message -- this might not involve testing
> nworkers_launched in the way parallel CREATE INDEX happens to.)
>

I have updated the comments atop WaitForParallelWorkersToAttach() to
address your above two points.

> known_started_workers looks a lot like any_message_received.  Perhaps 
> any_message_received should be renamed to known_started_workers and reused 
> here.  After all, if we know that a
> worker was started, there's no need for WaitForParallelWorkersToFinish to 
> again call GetBackgroundWorkerPid() for it.
>

Changed as per above suggestion.


> I think that you shouldn't need the 10ms delay loop; waiting forever should 
> work.
>

As discussed, I have changed the code as per your suggestion.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Attachment: WaitForParallelWorkersToAttach_v2.patch
Description: Binary data

Reply via email to