Обсуждение: Query running for very long time (server hanged) with parallel append

Thanks Rajkumar for catching this. I will have a look ...

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Feb 1, 2018 at 11:29 PM, Rajkumar Raghuwanshi
<rajkumar.raghuwanshi@enterprisedb.com> wrote:
> I am getting server hang kind of issue with the below postgres.conf setup.
> Issue may occur while running below query single/multiple times (random).
> Not getting terminal back even after cancelling query.
> explain output and query is given below.

Whatever logic bug might be causing the query to hang, it's not good
that we're unable to SIGINT/SIGTERM our way out of this state.  See
also this other bug report for a known problem (already fixed but not
yet released), but which came with an extra complaint, as yet
unexplained, that the query couldn't be interrupted:

https://www.postgresql.org/message-id/flat/151724453314.1238.409882538067070269%40wrigleys.postgresql.org

-- 
Thomas Munro
http://www.enterprisedb.com

On 2 February 2018 at 03:50, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
> Whatever logic bug might be causing the query to hang, it's not good
> that we're unable to SIGINT/SIGTERM our way out of this state.  See
> also this other bug report for a known problem (already fixed but not
> yet released), but which came with an extra complaint, as yet
> unexplained, that the query couldn't be interrupted:
>
> https://www.postgresql.org/message-id/flat/151724453314.1238.409882538067070269%40wrigleys.postgresql.org

Yeah, it is not good that there is no response to the SIGINT.

The query is actually hanging because one of the workers is in a small
loop where it iterates over the subplans searching for unfinished
plans, and it never comes out of the loop (it's a bug which I am yet
to fix). And it does not make sense to keep CHECK_FOR_INTERRUPTS in
each iteration; it's a small loop that does not pass control to any
other functions .

But I am not sure about this : while the workers are at it, why the
backend that is waiting for the workers does not come out of the wait
state with a SIGINT. I guess the same issue has been discussed in the
mail thread that you pointed.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

On Fri, Feb 2, 2018 at 1:43 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 2 February 2018 at 03:50, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
>> Whatever logic bug might be causing the query to hang, it's not good
>> that we're unable to SIGINT/SIGTERM our way out of this state.  See
>> also this other bug report for a known problem (already fixed but not
>> yet released), but which came with an extra complaint, as yet
>> unexplained, that the query couldn't be interrupted:
>>
>> https://www.postgresql.org/message-id/flat/151724453314.1238.409882538067070269%40wrigleys.postgresql.org
>
> Yeah, it is not good that there is no response to the SIGINT.
>
> The query is actually hanging because one of the workers is in a small
> loop where it iterates over the subplans searching for unfinished
> plans, and it never comes out of the loop (it's a bug which I am yet
> to fix). And it does not make sense to keep CHECK_FOR_INTERRUPTS in
> each iteration; it's a small loop that does not pass control to any
> other functions .

Uh, sounds like we'd better fix that bug.

> But I am not sure about this : while the workers are at it, why the
> backend that is waiting for the workers does not come out of the wait
> state with a SIGINT. I guess the same issue has been discussed in the
> mail thread that you pointed.

Is it getting stuck here?

    /*
     * We can't finish transaction commit or abort until all of the workers
     * have exited.  This means, in particular, that we can't respond to
     * interrupts at this stage.
     */
    HOLD_INTERRUPTS();
    WaitForParallelWorkersToExit(pcxt);
    RESUME_INTERRUPTS();

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

On 2 February 2018 at 20:46, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Feb 2, 2018 at 1:43 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>> The query is actually hanging because one of the workers is in a small
>> loop where it iterates over the subplans searching for unfinished
>> plans, and it never comes out of the loop (it's a bug which I am yet
>> to fix). And it does not make sense to keep CHECK_FOR_INTERRUPTS in
>> each iteration; it's a small loop that does not pass control to any
>> other functions .
>
> Uh, sounds like we'd better fix that bug.


The scenario is this : One of the workers w1 hasn't yet chosen the
first plan, and all the plans are already finished. So w1 has it's
node->as_whichplan equal to -1. So the below condition in
choose_next_subplan_for_worker() never becomes true for this worker :

if (pstate->pa_next_plan == node->as_whichplan)
{
   /* We've tried everything! */
   pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
   LWLockRelease(&pstate->pa_lock);
   return false;
}

What I think is : we should save the information about which plan we
started the search with, before the loop starts. So, we save the
starting plan value like this, before the loop starts:
   initial_plan = pstate->pa_next_plan;

And then break out of the loop when we come back to to this initial plan :
if (initial_plan == pstate->pa_next_plan)
   break;

Now, suppose it so happens that initial_plan is a non-partial plan.
And then when we wrap around to the first partial plan, we check
(initial_plan == pstate->pa_next_plan) which will never become true
because initial_plan is less than first_partial_plan.

So what I have done in the patch is : have a flag 'should_wrap_around'
indicating that we should not wrap around. This flag is true when
initial_plan is a non-partial plan, in which case we know that we will
have covered all plans by the time we reach the last plan. So break
from the loop if this flag is false, or if we have reached the initial
plan.

Attached is a patch that fixes this issue on the above lines.

>
>> But I am not sure about this : while the workers are at it, why the
>> backend that is waiting for the workers does not come out of the wait
>> state with a SIGINT. I guess the same issue has been discussed in the
>> mail thread that you pointed.
>
> Is it getting stuck here?
>
>     /*
>      * We can't finish transaction commit or abort until all of the workers
>      * have exited.  This means, in particular, that we can't respond to
>      * interrupts at this stage.
>      */
>     HOLD_INTERRUPTS();
>     WaitForParallelWorkersToExit(pcxt);
>     RESUME_INTERRUPTS();

Actually the backend is getting stuck in
choose_next_subplan_for_leader(), in LWLockAcquire(), waiting for the
hanging worker to release the pstate->pa_lock. I think there is
nothing wrong in this, because it is assumed that LWLock wait is going
to be for very short tiime, and because of this bug, the lwlock waits
forever.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

At Mon, 5 Feb 2018 15:29:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
<CAJ3gD9eFR8=kxjPYBEHe34uT9+RYET0VbhGEfSt79eZx3L9E1Q@mail.gmail.com>
> On 2 February 2018 at 20:46, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Fri, Feb 2, 2018 at 1:43 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> >> The query is actually hanging because one of the workers is in a small
> >> loop where it iterates over the subplans searching for unfinished
> >> plans, and it never comes out of the loop (it's a bug which I am yet
> >> to fix). And it does not make sense to keep CHECK_FOR_INTERRUPTS in
> >> each iteration; it's a small loop that does not pass control to any
> >> other functions .
> >
> > Uh, sounds like we'd better fix that bug.
> 
> 
> The scenario is this : One of the workers w1 hasn't yet chosen the
> first plan, and all the plans are already finished. So w1 has it's
> node->as_whichplan equal to -1. So the below condition in
> choose_next_subplan_for_worker() never becomes true for this worker :
> 
> if (pstate->pa_next_plan == node->as_whichplan)
> {
>    /* We've tried everything! */
>    pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
>    LWLockRelease(&pstate->pa_lock);
>    return false;
> }
> 
> What I think is : we should save the information about which plan we
> started the search with, before the loop starts. So, we save the
> starting plan value like this, before the loop starts:
>    initial_plan = pstate->pa_next_plan;
> 
> And then break out of the loop when we come back to to this initial plan :
> if (initial_plan == pstate->pa_next_plan)
>    break;
> 
> Now, suppose it so happens that initial_plan is a non-partial plan.
> And then when we wrap around to the first partial plan, we check
> (initial_plan == pstate->pa_next_plan) which will never become true
> because initial_plan is less than first_partial_plan.
> 
> So what I have done in the patch is : have a flag 'should_wrap_around'
> indicating that we should not wrap around. This flag is true when
> initial_plan is a non-partial plan, in which case we know that we will
> have covered all plans by the time we reach the last plan. So break
> from the loop if this flag is false, or if we have reached the initial
> plan.
> 
> Attached is a patch that fixes this issue on the above lines.

The patch adds two new variables and always sets them but warp
around can happen at most once per call so I think it is enough
to arrange to stop at the wrap around time. Addition to that the
patch is forgetting the case of no partial plan. The loop can
overrun on pa_finished in the case.

I think something like the following would work.

@@ -485,6 +485,13 @@ choose_next_subplan_for_worker(AppendState *node)
     {
       /* Loop back to first partial plan. */
       pstate->pa_next_plan = append->first_partial_plan;
+
+      /*
+       * Arrange to bail out if we are trying to fetch the first partial
+       * plan
+       */
+      if (node->as_whichplan < append->first_partial_plan)
+        node->as_whichplan = append->first_partial_plan;
     }
     else

> >> But I am not sure about this : while the workers are at it, why the
> >> backend that is waiting for the workers does not come out of the wait
> >> state with a SIGINT. I guess the same issue has been discussed in the
> >> mail thread that you pointed.
> >
> > Is it getting stuck here?
> >
> >     /*
> >      * We can't finish transaction commit or abort until all of the workers
> >      * have exited.  This means, in particular, that we can't respond to
> >      * interrupts at this stage.
> >      */
> >     HOLD_INTERRUPTS();
> >     WaitForParallelWorkersToExit(pcxt);
> >     RESUME_INTERRUPTS();
> 
> Actually the backend is getting stuck in
> choose_next_subplan_for_leader(), in LWLockAcquire(), waiting for the
> hanging worker to release the pstate->pa_lock. I think there is
> nothing wrong in this, because it is assumed that LWLock wait is going
> to be for very short tiime, and because of this bug, the lwlock waits
> forever.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 06 Feb 2018 13:34:19 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20180206.133419.02213593.horiguchi.kyotaro@lab.ntt.co.jp>
> At Mon, 5 Feb 2018 15:29:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
<CAJ3gD9eFR8=kxjPYBEHe34uT9+RYET0VbhGEfSt79eZx3L9E1Q@mail.gmail.com>
> > On 2 February 2018 at 20:46, Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Fri, Feb 2, 2018 at 1:43 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> > >> The query is actually hanging because one of the workers is in a small
> > >> loop where it iterates over the subplans searching for unfinished
> > >> plans, and it never comes out of the loop (it's a bug which I am yet
> > >> to fix). And it does not make sense to keep CHECK_FOR_INTERRUPTS in
> > >> each iteration; it's a small loop that does not pass control to any
> > >> other functions .
> > >
> > > Uh, sounds like we'd better fix that bug.
> > 
> > 
> > The scenario is this : One of the workers w1 hasn't yet chosen the
> > first plan, and all the plans are already finished. So w1 has it's
> > node->as_whichplan equal to -1. So the below condition in
> > choose_next_subplan_for_worker() never becomes true for this worker :
> > 
> > if (pstate->pa_next_plan == node->as_whichplan)
> > {
> >    /* We've tried everything! */
> >    pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
> >    LWLockRelease(&pstate->pa_lock);
> >    return false;
> > }
> > 
> > What I think is : we should save the information about which plan we
> > started the search with, before the loop starts. So, we save the
> > starting plan value like this, before the loop starts:
> >    initial_plan = pstate->pa_next_plan;
> > 
> > And then break out of the loop when we come back to to this initial plan :
> > if (initial_plan == pstate->pa_next_plan)
> >    break;
> > 
> > Now, suppose it so happens that initial_plan is a non-partial plan.
> > And then when we wrap around to the first partial plan, we check
> > (initial_plan == pstate->pa_next_plan) which will never become true
> > because initial_plan is less than first_partial_plan.
> > 
> > So what I have done in the patch is : have a flag 'should_wrap_around'
> > indicating that we should not wrap around. This flag is true when
> > initial_plan is a non-partial plan, in which case we know that we will
> > have covered all plans by the time we reach the last plan. So break
> > from the loop if this flag is false, or if we have reached the initial
> > plan.
> > 
> > Attached is a patch that fixes this issue on the above lines.
> 
> The patch adds two new variables and always sets them but warp
> around can happen at most once per call so I think it is enough
> to arrange to stop at the wrap around time. Addition to that the
> patch is forgetting the case of no partial plan. The loop can
> overrun on pa_finished in the case.

Sorry, the patch works fine. But still the new variables don't
seem needed.

> I think something like the following would work.
> 
> @@ -485,6 +485,13 @@ choose_next_subplan_for_worker(AppendState *node)
>      {
>        /* Loop back to first partial plan. */
>        pstate->pa_next_plan = append->first_partial_plan;
> +
> +      /*
> +       * Arrange to bail out if we are trying to fetch the first partial
> +       * plan
> +       */
> +      if (node->as_whichplan < append->first_partial_plan)
> +        node->as_whichplan = append->first_partial_plan;
>      }
>      else
> 
> > >> But I am not sure about this : while the workers are at it, why the
> > >> backend that is waiting for the workers does not come out of the wait
> > >> state with a SIGINT. I guess the same issue has been discussed in the
> > >> mail thread that you pointed.
> > >
> > > Is it getting stuck here?
> > >
> > >     /*
> > >      * We can't finish transaction commit or abort until all of the workers
> > >      * have exited.  This means, in particular, that we can't respond to
> > >      * interrupts at this stage.
> > >      */
> > >     HOLD_INTERRUPTS();
> > >     WaitForParallelWorkersToExit(pcxt);
> > >     RESUME_INTERRUPTS();
> > 
> > Actually the backend is getting stuck in
> > choose_next_subplan_for_leader(), in LWLockAcquire(), waiting for the
> > hanging worker to release the pstate->pa_lock. I think there is
> > nothing wrong in this, because it is assumed that LWLock wait is going
> > to be for very short tiime, and because of this bug, the lwlock waits
> > forever.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 6 February 2018 at 10:11, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> At Mon, 5 Feb 2018 15:29:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
>> > Attached is a patch that fixes this issue on the above lines.
>>
>> The patch adds two new variables and always sets them but warp
>> around can happen at most once per call so I think it is enough
>> to arrange to stop at the wrap around time. Addition to that the
>> patch is forgetting the case of no partial plan. The loop can
>> overrun on pa_finished in the case.
>
> Sorry, the patch works fine. But still the new variables don't
> seem needed.

True, I could have set initially as_whichplan to pa_next_plan, and
used as_whichplan instead of initial_plan. And, I could have used the
condition initial_plan >= append->first_partial_plan instead of
should_wrap_around. The reason I used these two variables is because I
thought  the logic would be more reader-friendly. (Also,
should_wrap_around uses static values so using this variable avoids
determining the condition (initial_plan >= append->first_partial_plan)
at each iteration).

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

At Tue, 6 Feb 2018 11:56:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
<CAJ3gD9fJhKBHn-fx6UzYZeLa0N8WnnZb2BsUTm_5A9kYsJgnww@mail.gmail.com>
> On 6 February 2018 at 10:11, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> At Mon, 5 Feb 2018 15:29:27 +0530, Amit Khandekar <amitdkhan.pg@gmail.com> wrote in
> >> > Attached is a patch that fixes this issue on the above lines.
> >>
> >> The patch adds two new variables and always sets them but warp
> >> around can happen at most once per call so I think it is enough
> >> to arrange to stop at the wrap around time. Addition to that the
> >> patch is forgetting the case of no partial plan. The loop can
> >> overrun on pa_finished in the case.
> >
> > Sorry, the patch works fine. But still the new variables don't
> > seem needed.
> 
> True, I could have set initially as_whichplan to pa_next_plan, and
> used as_whichplan instead of initial_plan. And, I could have used the
> condition initial_plan >= append->first_partial_plan instead of
> should_wrap_around. The reason I used these two variables is because I
> thought  the logic would be more reader-friendly. (Also,
> should_wrap_around uses static values so using this variable avoids
> determining the condition (initial_plan >= append->first_partial_plan)
> at each iteration).

Thanks for the explanation. I'm fine with it. Well may I make
some comments on the patch?

- It seems to me that the if (!should_warp_around) block and else
  block are better be transposed so that make the bail out code
  closer to the exit condition check as the previous code
  was. (In other was the second block should be if
  (should_wrap...), not if (!should_warp..).

- The following comment needs a "the" before the "first", maybe.
 +            /* Loop back to first partial plan. */


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 6 February 2018 at 16:11, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Thanks for the explanation. I'm fine with it. Well may I make
> some comments on the patch?

Sure, always welcome.

>
> - It seems to me that the if (!should_warp_around) block and else
>   block are better be transposed so that make the bail out code
>   closer to the exit condition check as the previous code
>   was. (In other was the second block should be if
>   (should_wrap...), not if (!should_warp..).

Yeah, I think it looks equally good that way, and like you said, the
current code does it that way. So in the attached patch, I have
swapped the two conditions.

>
> - The following comment needs a "the" before the "first", maybe.
>  +                      /* Loop back to first partial plan. */

Now since we are not changing this line in the patch, I have avoided
that. But the committer can very well make this change, all though I
am not sure myself about the grammar.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Feb 6, 2018 at 11:32 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> Yeah, I think it looks equally good that way, and like you said, the
> current code does it that way. So in the attached patch, I have
> swapped the two conditions.

I prefer to avoid introducing 2 new variables and instead just prevent
the looping directly in the case where we started with a non-partial
plan.

See attached.  Does this look OK?

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

At Tue, 6 Feb 2018 13:50:28 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoYqdC+9U8mLYkUgM=CaBt6Pzz4R_YNboqDbW-LvUaHO+g@mail.gmail.com>
> On Tue, Feb 6, 2018 at 11:32 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> > Yeah, I think it looks equally good that way, and like you said, the
> > current code does it that way. So in the attached patch, I have
> > swapped the two conditions.
> 
> I prefer to avoid introducing 2 new variables and instead just prevent
> the looping directly in the case where we started with a non-partial
> plan.
> 
> See attached.  Does this look OK?

Ah, we can bail out when starting from the first partial plan.
It's a bit uneasy that pa_next_plan can be -1 but it looks
perfect to me.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 7 February 2018 at 07:30, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Tue, 6 Feb 2018 13:50:28 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoYqdC+9U8mLYkUgM=CaBt6Pzz4R_YNboqDbW-LvUaHO+g@mail.gmail.com>
>> On Tue, Feb 6, 2018 at 11:32 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>> > Yeah, I think it looks equally good that way, and like you said, the
>> > current code does it that way. So in the attached patch, I have
>> > swapped the two conditions.
>>
>> I prefer to avoid introducing 2 new variables and instead just prevent
>> the looping directly in the case where we started with a non-partial
>> plan.
>>
>> See attached.  Does this look OK?
>
> Ah, we can bail out when starting from the first partial plan.


> It's a bit uneasy that pa_next_plan can be -1

I also feel that way. In the new condition (node->as_whichplan >
append->first_partial_plan), there is an implicit assumption that
INVALID_SUBPLAN_INDEX is equal to -1. So in our issue, if
node->as_whichplan is -1, this condition becomes false and so the loop
breaks correctly. But like I said, it works correctly because
INVALID_SUBPLAN_INDEX is defined to be less than zero. I guess, this
assumption might be safe in the long term also.

But another thing is that, node->as_whichplan can point to some
non-partial plan because it is the value of the earlier chosen plan.
And in that case (node->as_whichplan > append->first_partial_plan) is
false, indicating we should not wrap around. But there still might be
unfinished partial plans, and the pa_next_plan might be pointing to,
say, a plan at the tail end. Here, it still makes sense to wrap around
and see if there are unfinished partial plans. But the above condition
would not allow us to wrap around.

For this, we should check whether we have started with a partial plan.
That is, use this condition like how I used : (initial_plan >=
append->first_partial_plan) . Or else, if we don't want to add a new
variable, set node->as_whichplan to pa_next_plan initially.

Also, the first condition is not necessary here :
-       else if (append->first_partial_plan < node->as_nplans)
+       else if (append->first_partial_plan < node->as_nplans &&
+                node->as_whichplan > append->first_partial_plan)
Just the fact that we are starting from a non-partial plan is enough
to determine that we should not wrap around, regardless of whether
there are any partial plans.
So we can just keep this single condition :
-       else if (append->first_partial_plan < node->as_nplans)
+       else if (node->as_whichplan > append->first_partial_plan)

Attached is the patch accordingly.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

On Wed, Feb 7, 2018 at 2:11 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> Attached is the patch accordingly.

OK, I see.  That makes sense; committed.

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