Horton

Re: EINTR in ftruncate()

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Chris Travers <chris.travers@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>, Chris Travers <chris.travers@gmail.com>
Date: 2022-07-01T21:06:15Z
Lists: pgsql-hackers 
Hi Chris,

On 2022-07-01 13:29:44 -0700, Andres Freund wrote:
> On 2022-07-01 19:55:16 +0200, Alvaro Herrera wrote:
> > On 2022-Jul-01, Andres Freund wrote:
> > 
> > > On 2022-07-01 17:41:05 +0200, Alvaro Herrera wrote:
> > > > Nicola Contu reported two years ago to pgsql-general[1] that they were
> > > > having sporadic query failures, because EINTR is reported on some system
> > > > call.  I have been told that the problem persists, though it is very
> > > > infrequent.  I propose the attached patch.  Kyotaro proposed a slightly
> > > > different patch which also protects write(), but I think that's not
> > > > necessary.
> > > 
> > > What is the reason for the || ProcDiePending || QueryCancelPending bit? What
> > > if there's dsm operations intentionally done while QueryCancelPending?
> > 
> > That mirrors the test for the other block in that function, which was
> > added by 63efab4ca139, whose commit message explains:
> > 
> >     Allow DSM allocation to be interrupted.
> >     
> >     Chris Travers reported that the startup process can repeatedly try to
> >     cancel a backend that is in a posix_fallocate()/EINTR loop and cause it
> >     to loop forever.  Teach the retry loop to give up if an interrupt is
> >     pending.  Don't actually check for interrupts in that loop though,
> >     because a non-local exit would skip some clean-up code in the caller.
> 
> That whole approach seems quite wrong to me. At the absolute very least the
> code needs to check if interrupts are being processed in the current context
> before just giving up due to ProcDiePending || QueryCancelPending.
> 
> I'm very unconvinced this ought to be fixed in dsm_impl_posix_resize(), rather
> than the startup process signalling.
> 
> There is an argument for allowing more things to be cancelled, but we'd need a
> retry loop for the !INTERRUPTS_CAN_BE_PROCESSED() case.

Chris, do you have any additional details about the machine that lead to this
change? OS version, whether it might have been swapping, etc?

I wonder if what happened is that posix_fallocate() used glibc's fallback
implementation because the kernel was old enough to not support fallocate()
for tmpfs.  Looks like support for fallocate() for tmpfs was added in 3.5
([1]). So e.g. a rhel 6 wouldn't have had that.

Greetings,

Andres Freund

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2d12e22c59ce714008aa5266d769f8568d74eac

Commits

  1. Provide sigaction() for Windows.

  2. Emulate sigprocmask(), not sigsetmask(), on Windows.

  3. Make dsm_impl_posix_resize more future-proof.

  4. Don't clobber postmaster sigmask in dsm_impl_resize.

  5. Create a distinct wait event for POSIX DSM allocation.

  6. Remove redundant ftruncate() for POSIX DSM memory.

  7. Block signals while allocating DSM memory.

  8. Remove dsm_resize() and dsm_remap().

  9. XLOG (also known as WAL -:)) Bootstrap/Startup/Shutdown.