Обсуждение: Funny hang on PostgreSQL 10 during parallel index scan on slave

Hi,

On 2018-09-05 18:48:44 +0200, Chris Travers wrote:
> Will submit a patch here shortly.  Thanks!  Should we do for master and
> 10?  Or 9.6 too?

Please don't top-post on this list.  This needs to be done in all
branches where the posix_fallocate call is present.

> > Yep,  Maybe we should check for signals there.
> >
> > On Wed, Sep 5, 2018 at 5:27 PM Thomas Munro <thomas.munro@enterprisedb.com>
> > wrote:
> >
> >> On Wed, Sep 5, 2018 at 8:23 AM Chris Travers <chris.travers@adjust.com>
> >> wrote:
> >> > 1.  The query is in a parallel index scan or similar
> >> > 2.  A process is executing a parallel plan and allocating a significant
> >> chunk of memory (2MB for example) in dynamic shared memory.
> >> > 3.  The startup process goes into a loop where it sends a sigusr1,
> >> sleeps 5m, and sends another sigusr1 etc.
> >> > 4.  The sigusr1 aborts the system call, which is then retried.
> >> > 5.  Because the system call takes more than 5ms, we end up in an
> >> endless loop

What you're presumably encountering here is a recovery conflict.

> On Wed, Sep 5, 2018 at 6:40 PM Chris Travers <chris.travers@adjust.com>
> wrote:
> >> Do you mean this loop in dsm_impl_posix_resize() is getting
> >> interrupted constantly and never completing?
> >>
> >>                 /* We may get interrupted, if so just retry. */
> >>                 do
> >>                 {
> >>                         rc = posix_fallocate(fd, 0, size);
> >>                 } while (rc == EINTR);
> >>

Probably worthwile to check that the dsm code is properly robust if
errors are thrown from within here.

Greetings,

Andres Freund

Ok, so here's my current patch (work in progress).  I have not run tests yet, and finding a way to add a test case is virtually impossible though I expect we will find ways of using gdb to confirm local fix later.

After careful code review, I settled on the following approach which seemed to be the least intrusive.

1.  Change the retry logic so that it does not retry of QueryCancelPending or ProcDiePending are set.  In these cases EINTR is passed back to the caller

2.  Change the error handling logic of the caller so that EINTR is handled by the next CHECK_FOR_INTERRUPTS() after cleanup.

This means that the file descriptor is now already closed and that we handle this the same way we would with a ENOSPC.  The reason for this is that there are many places in the code where it is not clear what degree of safety is present for throwing errors using ereport, and the patch needs to be as unobtrusive as possible to facilitate back porting.

At this point the patch is for discussion.  I have not even compiled it or tested it but maybe there is something wrong with my approach so figured I would send it out early.

The major assumptions are:

1.  close() will never take longer than the interrupt interval that caused the loop to break.

2.  close() does not get interrupted in a way that will not cause cleanup problems later.

3. We will reach the next interrupt check at a reasonable interval and safe spot.

Any initial arguments against?

--

Best Regards,

Chris Travers

Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com 

Saarbrücker Straße 37a, 10405 Berlin

On Thu, Sep 6, 2018 at 11:08 AM Chris Travers <chris.travers@adjust.com> wrote:

Ok, so here's my current patch (work in progress).  I have not run tests yet, and finding a way to add a test case is virtually impossible though I expect we will find ways of using gdb to confirm local fix later.

After careful code review, I settled on the following approach which seemed to be the least intrusive.

1.  Change the retry logic so that it does not retry of QueryCancelPending or ProcDiePending are set.  In these cases EINTR is passed back to the caller

2.  Change the error handling logic of the caller so that EINTR is handled by the next CHECK_FOR_INTERRUPTS() after cleanup.

This means that the file descriptor is now already closed and that we handle this the same way we would with a ENOSPC.  The reason for this is that there are many places in the code where it is not clear what degree of safety is present for throwing errors using ereport, and the patch needs to be as unobtrusive as possible to facilitate back porting.

At this point the patch is for discussion.  I have not even compiled it or tested it but maybe there is something wrong with my approach so figured I would send it out early.

The major assumptions are:

1.  close() will never take longer than the interrupt interval that caused the loop to break.

2.  close() does not get interrupted in a way that will not cause cleanup problems later.

3. We will reach the next interrupt check at a reasonable interval and safe spot.

Any initial arguments against?

The previous patch had two issues found on internal code review here.  I am sending a new patch.

This patch compiles.  make check passes

make check-world fails but the errors are the same as found on master and involve ecpg.

We will be doing further internal review and verification and then will run through pg_indent before final submission.

Changes in this patch:

1.  Previous patch had backwards check for EINTR in calling function.  This was fixed.

2.  In cases where ERROR elevel was passed in, function would return instead of error out on case of error.

So in this case we check if we expect to error on error and if so, check for interrupts.  If not, we go through the normal error handling/logging path which might result in an warning that shared memory segment could not be allocated followed by an actual meaningful error.  I could put that in an else if, if that is seen as a good idea, but figured I would raise it as a discussion point.

 

--

Best Regards,

Chris Travers

Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com 

Saarbrücker Straße 37a, 10405 Berlin

--

Best Regards,

Chris Travers

Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com 

Saarbrücker Straße 37a, 10405 Berlin