Excessive PostmasterIsAlive calls slow down WAL redo

Heikki Linnakangas wrote:

That seems like an utter waste of time. I'm almost inclined to call that a
performance bug. As a straightforward fix, I'd suggest that we call
HandleStartupProcInterrupts() in the WAL redo loop, not on every record, but
only e.g. every 32 records. That would make the main redo loop less
responsive to shutdown, SIGHUP, or postmaster death, but that seems OK.
There are also calls to HandleStartupProcInterrupts() in the various other
loops, that wait for new WAL to arrive or recovery delay, so this would only
affect the case where we're actively replaying records.

I think calling PostmasterIsAlive only every 32 records is sensible, but
I'm not so sure about the other two things happening in
HandleStartupProcInterrupts (which are much cheaper anyway, since they
only read a local flag); if records are large or otherwise slow to
replay, I'd rather not wait for 32 of them before the process honoring
my shutdown request. Why not split the function in two, or maybe add a
flag "check for postmaster alive too", passed as true every 32 records?

The lesson seems to be that PostmasterIsAlive is moderately expensive
(one syscall). It's also used in WaitLatchOrSocket when
WL_POSTMASTER_DEATH is given. I didn't audit its callers terribly
carefully but I think these uses are not as performance-sensitive.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5 April 2018 at 08:23, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

That seems like an utter waste of time. I'm almost inclined to call that a
performance bug. As a straightforward fix, I'd suggest that we call
HandleStartupProcInterrupts() in the WAL redo loop, not on every record, but
only e.g. every 32 records. That would make the main redo loop less
responsive to shutdown, SIGHUP, or postmaster death, but that seems OK.
There are also calls to HandleStartupProcInterrupts() in the various other
loops, that wait for new WAL to arrive or recovery delay, so this would only
affect the case where we're actively replaying records.

+1

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2018-04-05 10:23:43 +0300, Heikki Linnakangas wrote:

Profiling that, without any patches applied, I noticed that a lot of time
was spent in read()s on the postmaster-death pipe, i.e. in
PostmasterIsAlive(). We call that between *every* WAL record.

That seems like an utter waste of time. I'm almost inclined to call that a
performance bug. As a straightforward fix, I'd suggest that we call
HandleStartupProcInterrupts() in the WAL redo loop, not on every record, but
only e.g. every 32 records.

I agree this is a performance problem. I do however not like the fix.
ISTM the better approach would be to try to reduce the cost of
PostmasterIsAlive() on common platforms - it should be nearly free if
done right.

One way to achieve that would e.g. to stop ignoring SIGPIPE and instead
check for postmaster death inside the handler, without reacting to
it. Then the the actual PostmasterIsAlive() checks are just a check of a
single sig_atomic_t.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

ISTM the better approach would be to try to reduce the cost of
PostmasterIsAlive() on common platforms - it should be nearly free if
done right.

+1 if it's doable.

One way to achieve that would e.g. to stop ignoring SIGPIPE and instead
check for postmaster death inside the handler, without reacting to
it. Then the the actual PostmasterIsAlive() checks are just a check of a
single sig_atomic_t.

AFAIR, we do not get SIGPIPE on the postmaster pipe, because nobody
ever writes to it. So this sketch seems off to me, even assuming that
not-ignoring SIGPIPE causes no problems elsewhere.

While it's not POSIX, at least some platforms are capable of delivering
a separate signal on parent process death. Perhaps using that where
available would be enough of an answer.

regards, tom lane

On 2018-04-05 14:39:27 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

ISTM the better approach would be to try to reduce the cost of
PostmasterIsAlive() on common platforms - it should be nearly free if
done right.

+1 if it's doable.

One way to achieve that would e.g. to stop ignoring SIGPIPE and instead
check for postmaster death inside the handler, without reacting to
it. Then the the actual PostmasterIsAlive() checks are just a check of a
single sig_atomic_t.

AFAIR, we do not get SIGPIPE on the postmaster pipe, because nobody
ever writes to it. So this sketch seems off to me, even assuming that
not-ignoring SIGPIPE causes no problems elsewhere.

Yea, you're probably right. I'm mostly brainstorming here.

(FWIW, I don't think not ignoring SIGPIPE would be a huge issue if we
don't immediately take any action on its account)

While it's not POSIX, at least some platforms are capable of delivering
a separate signal on parent process death. Perhaps using that where
available would be enough of an answer.

Yea, that'd work on linux. Which is probably the platform 80-95% of
performance critical PG workloads run on. There's
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on windows, which might also work,
but I'm not sure it provides enough opportunity for cleanup.

Greetings,

Andres Freund

On Thu, Apr 05, 2018 at 02:39:27PM -0400, Tom Lane wrote:

While it's not POSIX, at least some platforms are capable of delivering
a separate signal on parent process death. Perhaps using that where
available would be enough of an answer.

Are you referring to prctl here?

+1 on improving performance of PostmasterIsAlive() if possible instead
of checking for it every N records.  You barely see records creating a
database from a large template close to each other but that could hurt
the response time of the redo process.
--
Michael

Greetings,

* Andres Freund (andres@anarazel.de) wrote:

On 2018-04-05 14:39:27 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

ISTM the better approach would be to try to reduce the cost of
PostmasterIsAlive() on common platforms - it should be nearly free if
done right.

+1 if it's doable.

[...]

While it's not POSIX, at least some platforms are capable of delivering
a separate signal on parent process death. Perhaps using that where
available would be enough of an answer.

Yea, that'd work on linux. Which is probably the platform 80-95% of
performance critical PG workloads run on. There's
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on windows, which might also work,
but I'm not sure it provides enough opportunity for cleanup.

While I tend to agree that it'd be nice to just make it cheaper, that
doesn't seem like something that we'd be likely to back-patch and I tend
to share Heikki's feelings that this is a performance regression we
should be considering fixing in released versions.

What Alvaro posted up-thread seems like it might be a small enough
change to still be reasonable to back-patch and we can still think about
ways to make PostmasterIsAlive() cheaper in the future.

Thanks!

Stephen

Hi,

On 2018-04-06 07:39:28 -0400, Stephen Frost wrote:

While I tend to agree that it'd be nice to just make it cheaper, that
doesn't seem like something that we'd be likely to back-patch and I tend
to share Heikki's feelings that this is a performance regression we
should be considering fixing in released versions.

I'm doubtful about fairly characterizing this as a performance bug. It's
not like we've O(n^2) behaviour on our hand, and if your replay isn't of
a toy workload normally that one syscall isn't going to make a huge
difference because you've actual IO and such going on.

I'm also doubtful that it's sane to just check every 32 records. There's
records that can take a good chunk of time, and just continuing for
another 31 records seems like a bad idea.

Greetings,

Andres Freund

On 06/04/18 19:39, Andres Freund wrote:

On 2018-04-06 07:39:28 -0400, Stephen Frost wrote:

While I tend to agree that it'd be nice to just make it cheaper, that
doesn't seem like something that we'd be likely to back-patch and I tend
to share Heikki's feelings that this is a performance regression we
should be considering fixing in released versions.

To be clear, this isn't a performance *regression*. It's always been bad.

I'm not sure if I'd backpatch this. Maybe after it's been in 'master'
for a while and we've gotten some field testing of it.

I'm doubtful about fairly characterizing this as a performance bug. It's
not like we've O(n^2) behaviour on our hand, and if your replay isn't of
a toy workload normally that one syscall isn't going to make a huge
difference because you've actual IO and such going on.

If all the data fits in the buffer cache, then there would be no I/O.
Think of a smallish database that's heavily updated. There are a lot of
real applications like that.

I'm also doubtful that it's sane to just check every 32 records. There's
records that can take a good chunk of time, and just continuing for
another 31 records seems like a bad idea.

It's pretty arbitrary, I admit. It's the best I could come with, though.
If we could get a signal on postmaster death, that'd be best, but that's
a much bigger patch, and I'm worried that it would bring new portability
and reliability issues.

I'm not too worried about 32 records being too long an interval. True,
replaying 32 CREATE DATABASE records would take a long time. But pretty
much all other WAL records are fast enough to apply. We could make it
every 8 records rather than 32, if that makes you feel better. Or add
some extra conditions, like always check it when stepping to a new WAL
segment. In any case, the fundamental difference would be though to not
check it between every record.

- Heikki

Greetings,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:

On 06/04/18 19:39, Andres Freund wrote:

On 2018-04-06 07:39:28 -0400, Stephen Frost wrote:

While I tend to agree that it'd be nice to just make it cheaper, that
doesn't seem like something that we'd be likely to back-patch and I tend
to share Heikki's feelings that this is a performance regression we
should be considering fixing in released versions.

To be clear, this isn't a performance *regression*. It's always been bad.

Oh, I see, apologies for the confusion, my initial read was that this
was due to some patch that had gone in previously, hence it was an
actual regression. I suppose I tend to view performance issues as
either "regression" or "opportunity for improvement" and when you said
"bug" it made me think it was a regression. :)

I'm not sure if I'd backpatch this. Maybe after it's been in 'master' for a
while and we've gotten some field testing of it.

If it's not a regression then there's I definitely think the bar is much
higher to consider this something to back-patch. I wouldn't typically
argue for back-patching a performance improvement unless it's to address
a specific regression.

Thanks!

Stephen

Hi,

On 2018-04-05 12:20:38 -0700, Andres Freund wrote:

While it's not POSIX, at least some platforms are capable of delivering
a separate signal on parent process death. Perhaps using that where
available would be enough of an answer.

Yea, that'd work on linux. Which is probably the platform 80-95% of
performance critical PG workloads run on. There's
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on windows, which might also work,
but I'm not sure it provides enough opportunity for cleanup.

I coincidentally got pinged about our current approach causing
performance problems on FreeBSD and started writing a patch. The
problem there appears to be that constantly attaching events to the read
pipe end, from multiple processes, causes significant contention inside
the kernel. Which isn't that surprising. That's distinct from the
problem netbsd/openbsd reported a while back (superflous wakeups).

That person said he'd work on adding an equivalent of linux'
prctl(PR_SET_PDEATHSIG) to FreeBSD.

It's not particularly hard to whip something up that kind of
works. Getting some of the details right isn't entirely as clear
however:

It's trivial to make PostmasterIsAlive() cheap. In my prototype I've
setup PR_SET_PDEATHSIG to deliver SIGINFO to postmaster children. That
sets parent_potentially_died to true. PostmasterIsAlive() only runs the
main body when it's true, making it very cheap in the common case.

What I'm however not quite as clear on is how to best change
WL_POSTMASTER_DEATH.

One appraoch approach I can come up with is to use the self-pipe for
both WL_POSTMASTER_DEATH and WL_LATCH_SET. As we can cheaply check if
either set->latch->is_set or parent_potentially_died, re-using the
selfpipe works well enough. What I'm however not quite sure about is
how to best do so with epoll() - there we explicitly do not iterate over
all registered events, but use epoll_event->data to get just the wait
event associated with a readyness event. Therefor we've to keep track
of whether a WaitEventSet has both WL_POSTMASTER_DEATH and WL_LATCH_SET
registered, and check in both WL_POSTMASTER_DEATH/WL_LATCH_SET whether
the event is being waited for.

Another approach, that's simpler to implement, is to simply have a
second selfpipe, just for WL_POSTMASTER_DEATH.

A third question is whether we want to keep postmaster_alive_fds if we
have PR_SET_PDEATHSIG. I'd want to continue re-checking that the parent
is actually dead, but we could also do so by kill()ing postmaster like
we used to do before 89fd72cbf26f5d2e3d86ab19c1ead73ab8fac0fe. It's not
bad to get rid of unnecessary filedescriptors. Would imply a semantic
change however.

Greetings,

Andres Freund

Andres Freund wrote:

Another approach, that's simpler to implement, is to simply have a
second selfpipe, just for WL_POSTMASTER_DEATH.

Would it work to use this second pipe, to which each child writes a byte
that postmaster never reads, and then rely on SIGPIPE when postmaster
dies? Then we never need to do a syscall.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On April 9, 2018 6:31:07 PM PDT, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Andres Freund wrote:

Another approach, that's simpler to implement, is to simply have a
second selfpipe, just for WL_POSTMASTER_DEATH.

Would it work to use this second pipe, to which each child writes a
byte
that postmaster never reads, and then rely on SIGPIPE when postmaster
dies? Then we never need to do a syscall.

I'm not following, could you expand on what you're suggesting? Note that you do not get SIGPIPE for already buffered writes. Which syscall can we avoid?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Tue, Apr 10, 2018 at 12:53 PM, Andres Freund <andres@anarazel.de> wrote:

I coincidentally got pinged about our current approach causing
performance problems on FreeBSD and started writing a patch. The
problem there appears to be that constantly attaching events to the read
pipe end, from multiple processes, causes significant contention inside
the kernel. Which isn't that surprising. That's distinct from the
problem netbsd/openbsd reported a while back (superflous wakeups).

That person said he'd work on adding an equivalent of linux'
prctl(PR_SET_PDEATHSIG) to FreeBSD.

Just an idea, not tested: what about a reusable WaitEventSet with zero
timeout? Using the kqueue patch, that'd call kevent() which'd return
immediately and tell you if any postmaster death notifications had
arrive on your queue since last time you asked. It doesn't even touch
the pipe, or any other kernel objects apart from your own queue IIUC.

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

On April 9, 2018 6:36:19 PM PDT, Thomas Munro <thomas.munro@enterprisedb.com> wrote:

On Tue, Apr 10, 2018 at 12:53 PM, Andres Freund <andres@anarazel.de>
wrote:

I coincidentally got pinged about our current approach causing
performance problems on FreeBSD and started writing a patch. The
problem there appears to be that constantly attaching events to the

read

pipe end, from multiple processes, causes significant contention

inside

the kernel. Which isn't that surprising. That's distinct from the
problem netbsd/openbsd reported a while back (superflous wakeups).

That person said he'd work on adding an equivalent of linux'
prctl(PR_SET_PDEATHSIG) to FreeBSD.

Just an idea, not tested: what about a reusable WaitEventSet with zero
timeout? Using the kqueue patch, that'd call kevent() which'd return
immediately and tell you if any postmaster death notifications had
arrive on your queue since last time you asked. It doesn't even touch
the pipe, or any other kernel objects apart from your own queue IIUC.

We still create a lot of WES adhoc in a number of places. I don't think this would solve that? The problem isn't that IsAlive is expensive, it's that polling is expensive. It's possible that using kqueue would fix that, because the highest frequency cases use a persistent WES.

Andres

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Andres Freund wrote:

On April 9, 2018 6:31:07 PM PDT, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Would it work to use this second pipe, to which each child writes a
byte that postmaster never reads, and then rely on SIGPIPE when
postmaster dies? Then we never need to do a syscall.

I'm not following, could you expand on what you're suggesting? Note
that you do not get SIGPIPE for already buffered writes. Which
syscall can we avoid?

Ah. I was thinking we'd get SIGPIPE from the byte sent at the start, as
soon as the kernel saw that postmaster abandoned the fd by dying.
Scratch that then.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On April 9, 2018 6:57:23 PM PDT, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Andres Freund wrote:

On April 9, 2018 6:31:07 PM PDT, Alvaro Herrera

<alvherre@alvh.no-ip.org> wrote:

Would it work to use this second pipe, to which each child writes a
byte that postmaster never reads, and then rely on SIGPIPE when
postmaster dies? Then we never need to do a syscall.

I'm not following, could you expand on what you're suggesting? Note
that you do not get SIGPIPE for already buffered writes. Which
syscall can we avoid?

Ah. I was thinking we'd get SIGPIPE from the byte sent at the start,
as
soon as the kernel saw that postmaster abandoned the fd by dying.
Scratch that then.

Had the same idea, but unfortunately reality, in the form of a test program, cured me of my hope ;)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On 10/04/18 04:36, Thomas Munro wrote:

On Tue, Apr 10, 2018 at 12:53 PM, Andres Freund <andres@anarazel.de> wrote:

I coincidentally got pinged about our current approach causing
performance problems on FreeBSD and started writing a patch. The
problem there appears to be that constantly attaching events to the read
pipe end, from multiple processes, causes significant contention inside
the kernel. Which isn't that surprising. That's distinct from the
problem netbsd/openbsd reported a while back (superflous wakeups).

That person said he'd work on adding an equivalent of linux'
prctl(PR_SET_PDEATHSIG) to FreeBSD.

Just an idea, not tested: what about a reusable WaitEventSet with zero
timeout? Using the kqueue patch, that'd call kevent() which'd return
immediately and tell you if any postmaster death notifications had
arrive on your queue since last time you asked. It doesn't even touch
the pipe, or any other kernel objects apart from your own queue IIUC.

Hmm. In PostmasterIsAlive(), you'd still need to call kevent() to check
if postmaster has died. It would just replace the current read() syscall
on the pipe with the kevent() syscall. Is it faster?

- Heikki

On Wed, Apr 11, 2018 at 10:22 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 10/04/18 04:36, Thomas Munro wrote:

Just an idea, not tested: what about a reusable WaitEventSet with zero
timeout? Using the kqueue patch, that'd call kevent() which'd return
immediately and tell you if any postmaster death notifications had
arrive on your queue since last time you asked. It doesn't even touch
the pipe, or any other kernel objects apart from your own queue IIUC.

Hmm. In PostmasterIsAlive(), you'd still need to call kevent() to check if
postmaster has died. It would just replace the current read() syscall on the
pipe with the kevent() syscall. Is it faster?

It should be (based on the report of read() being slow here because of
contention on the pipe itself, I guess because of frequent poll() in
WaitLatch() elsewhere?).

But as I said over on another thread[1]/messages/by-id/CAEepm=298omvRS2C8WO=Cxp+WcM-Vn8V3x4_QhxURhKTRCSfYg@mail.gmail.com (sorry, it got tangled up with
that other conversation about a related topic), maybe testing
getppid() would be simpler and about as fast as possible given you
have to make a syscall (all processes should normally be children of
postmaster, right?). And only check every nth time through the loop,
as you said, to avoid high frequency syscalls. I think I might have
been guilty of having a solution looking for a problem, there ;-)

[1]: /messages/by-id/CAEepm=298omvRS2C8WO=Cxp+WcM-Vn8V3x4_QhxURhKTRCSfYg@mail.gmail.com

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