From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Chris Travers <chris(dot)travers(at)gmail(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Chris Travers <chris(dot)travers(at)gmail(dot)com> |
Subject: | Re: EINTR in ftruncate() |
Date: | 2022-07-01 21:06:15 |
Message-ID: | 20220701210615.m3mi52rhps3cdorq@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Nathan Bossart | 2022-07-01 21:12:25 | Re: replacing role-level NOINHERIT with a grant-level option |
Previous Message | Jacob Champion | 2022-07-01 20:59:42 | Re: [PATCH] Log details for client certificate failures |