Re: pgsql: Support Parallel Append plan nodes.

From: amul sul <sulamul(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <rhaas(at)postgresql(dot)org>, pgsql-committers(at)postgresql(dot)org
Subject: Re: pgsql: Support Parallel Append plan nodes.
Date: 2017-12-06 09:01:24
Message-ID: CAAJ_b95zsdBT2jNJuvs6GL3g29EXvSO+q4pxpuQ5mAhf5DTSBw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

Copying & reverting to Amit Khandekar's email here:

On Wed, Dec 6, 2017 at 11:45 AM, amul sul <sulamul(at)gmail(dot)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

Regards,
Amul

Attachment Content-Type Size
Parallel-Append-crash-fix-v2.patch application/octet-stream 1.6 KB

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Amit Khandekar 2017-12-06 10:01:44 Re: pgsql: Support Parallel Append plan nodes.
Previous Message Amit Khandekar 2017-12-06 08:43:49 Re: pgsql: Support Parallel Append plan nodes.

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2017-12-06 09:16:16 Re: es_query_dsa is broken
Previous Message Andres Freund 2017-12-06 08:03:19 Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple