On 6 December 2017 at 14:31, amul sul <sulamul@gmail.com> wrote:
> Copying & reverting to Amit Khandekar's email here:
>
> On Wed, Dec 6, 2017 at 11:45 AM, amul sul <sulamul@gmail.com> wrote:
>>> Thanks Tom for the crash analysis, I think this is the same reason that
>>> Rajkumar's reported case[1] was crashing only with partition-wise-join = on.
>>> I tried to fix this crash[2] but missed the place where I have added assert
>>> check in the assumption that we always coming from the previous check in the
>>> while loop.
>>>
>>> Instead, assert check we need a similar bailout logic[2] before looping back to
>>> first partial plan. Attached patch does the same, I've tested with
>>> parallel_leader_participation = off setting as suggested by Andres, make check
>>> looks good except there is some obvious regression diff.
>>>
>>> 1] https://postgr.es/m/CAKcux6m+6nTO6C08kKaj-Waffvgvp-9SD3RCGStX=Mzk0gQU8g@mail.gmail.com
>>> 2] https://postgr.es/m/CAAJ_b975k58H+Ed4=p0vbJunwO2reOMX5CVB8_R=JmXxY3uW=Q@mail.gmail.com
>>>
>>
>> @@ -506,7 +506,14 @@ choose_next_subplan_for_worker(AppendState *node)
>> node->as_whichplan = pstate->pa_next_plan++;
>> if (pstate->pa_next_plan >= node->as_nplans)
>> {
>> - Assert(append->first_partial_plan < node->as_nplans);
>> + /* No partial plans then bail out. */
>> + if (append->first_partial_plan >= node->as_nplans)
>> + {
>> + pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
>> + node->as_whichplan = INVALID_SUBPLAN_INDEX;
>> + LWLockRelease(&pstate->pa_lock);
>> + return false;
>> + }
>> pstate->pa_next_plan = append->first_partial_plan;
>>
>> In the above code, the fact that we have not bailed out from the
>> earlier for loop means that we have already found an unfinished plan
>> and node->as_whichplan is set to that plan. So while setting the next
>> plan above for the other workers to pick, we should not return false,
>> nor should we set node->as_whichplan to INVALID_SUBPLAN_INDEX.
>> Instead, just set pa_next_plan to INVALID_SUBPLAN_INDEX. Otherwise,
>> the chosen plan won't get executed at all.
>>
>
> Understood, thanks for the review. Updated patch attached.
>
> 1] https://postgr.es/m/CAJ3gD9e3_D3fFqzWBFYoaF-6yCXgbOFn3Mb-pgd_mxvjpsw7Rw@mail.gmail.com
This looks good.
In attached revised patch, just added some comments in the changes that you did.
>
> Regards,
> Amul
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company