Re: [HACKERS] Parallel Append implementation

From: amul sul <sulamul(at)gmail(dot)com>
To: Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Parallel Append implementation
Date: 2017-11-28 11:02:59
Message-ID: CAAJ_b975k58H+Ed4=p0vbJunwO2reOMX5CVB8_R=JmXxY3uW=Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Mon, Nov 27, 2017 at 10:21 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
> Thanks a lot Rajkumar for this test. I am able to reproduce this crash by
> enabling partition wise join.
>
> The reason for this crash is the same as
> the
> previous[1] i.e node->as_whichplan
> value. This time append->first_partial_plan value looks suspicious. With
> the
> following change to the v21 patch, I am able to reproduce this crash as
> assert
> failure when enable_partition_wise_join = ON otherwise working fine.
>
> diff --git a/src/backend/executor/nodeAppend.c
> b/src/backend/executor/nodeAppend.c
> index e3b17cf0e2..4b337ac633 100644
> --- a/src/backend/executor/nodeAppend.c
> +++ b/src/backend/executor/nodeAppend.c
> @@ -458,6 +458,7 @@ choose_next_subplan_for_worker(AppendState *node)
>
> /* Backward scan is not supported by parallel-aware plans */
> Assert(ScanDirectionIsForward(node->ps.state->es_direction));
> + Assert(append->first_partial_plan < node->as_nplans);
>
> LWLockAcquire(&pstate->pa_lock, LW_EXCLUSIVE);
>
>
> Will look into this more, tomorrow.
>
I haven't reached the actual reason why there wasn't any partial plan
(i.e. value of append->first_partial_plan & node->as_nplans are same)
when the partition-wise join is enabled. I think in this case we could simply
return false from choose_next_subplan_for_worker() when there aren't any
partial plan and we done with all non-partition plan, although I may be wrong
because I am yet to understand this patch.

Here are the changes I did on v21 patch to handle crash reported by Rajkumar[1]:

diff --git a/src/backend/executor/nodeAppend.c
b/src/backend/executor/nodeAppend.c
index e3b17cf0e2..e0ee918808 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -479,9 +479,12 @@ choose_next_subplan_for_worker(AppendState *node)
pstate->pa_next_plan = append->first_partial_plan;
else
pstate->pa_next_plan++;
- if (pstate->pa_next_plan == node->as_whichplan)
+
+ if (pstate->pa_next_plan == node->as_whichplan ||
+ (pstate->pa_next_plan == append->first_partial_plan &&
+ append->first_partial_plan >= node->as_nplans))
{
- /* We've tried everything! */
+ /* We've tried everything or there were no partial plans */
pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
LWLockRelease(&pstate->pa_lock);
return false;

Apart from this I have added few assert to keep eye on node->as_whichplan
value in the attached patch, thanks.

1] http://postgr.es/m/CAKcux6nyDxOyE4PA8O%3DQgF-ugZp_y1G2U%2Burmf76-%3Df2knDsWA%40mail.gmail.com

Regards,
Amul

Attachment Content-Type Size
ParallelAppend_v22.patch application/octet-stream 60.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2017-11-28 11:07:53 Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki
Previous Message Jan Michálek 2017-11-28 11:01:45 Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki