On 2017-06-15 15:06:43 -0700, Jeff Janes wrote:
> > Well, wal_writer_delay doesn't work if walwriter is in sleep mode, and
> > this afaics would allow wal writer to go into sleep mode with half a
> > page filled, and it'd not be woken up again until the page is filled.
> > No?
> >
> If it is taking the big sleep, then we always wake it up, since
> acd4c7d58baf09f.
> I didn't change that part.  I only changed what we do if it not hibernating.

Right, I hadn't read enough of the context.

> It looks like only limited consolidation was happening, the number of kills
> invoked was more than half of the number of SetLatch.  I think the reason
> is what when kill(owner_pid, SIGUSR1) is called, the kernel immediately
> suspends the calling process and gives the signalled process a chance to
> run in that time slice.  The Wal Writer gets woken up, sees that it only
> has a partial page to write and decides not to do anything, but resets the
> latch.  It does this fast enough that the subscription worker hasn't
> migrated CPUs yet.

I think part of the problem here is that latches signal the owning
process even if the owning process isn't actually sleeping - that's
obviously quite pointless.  In cases where walwriter is busy, that
actually causes a lot of superflous interrupted syscalls, because it
actually never ends up waiting on the latch. There's also a lot of
superflous context switches.  I think we can avoid doing so quite
easily, as e.g. with the attached patch.  Could you check how much that
helps your benchmark?

> The first change made the WALWriter wake up and do a write and flush
> whenever an async commit occurred and there was a completed WAL page to be
> written.  This way the hint bits could be set while the heap page was still
> hot, because they couldn't get set until the WAL covering the hinted-at
> transaction commit is flushed.
> The second change said we can set hint bits before the WAL covering the
> hinted-at transaction is flushed, as long the page LSN is newer than that
> (so the wal covering the hinted-at transaction commit must be flushed
> before the page containing the hint bit can be written).
> Then the third change makes the wal writer only write the full pages most
> of the times it is woken up, not flush them.  It only flushes them once
> every wal_writer_delay or wal_writer_flush_after, regardless of how often
> it is woken up.
> It seems like the third change rendered the first one useless.

I don't think so. Isn't the walwriter writing out the contents of the
WAL is quite important because it makes room in wal_buffers for new WAL?

I suspect we actually should wake up wal-writer *more* aggressively, to
offload wal fsyncs from individual backends, even when they're not
committing.  Right now we fsync whenever a segment is finished - we
really don't want to do that in backends that could do other useful
work.  I suspect it'd be a good idea to remove that logic from
XLogWrite() and replace it with
        if (ProcGlobal->walwriterLatch)

> Wouldn't it
> better, and much simpler, just to have reverted the first change once the
> second one was done?

I think both can actually happen independently, no? It's pretty common
for the page lsn to be *older* than the corresponding commit.  In fact
you'll always hit that case unless there's also concurrent writes also
touching said page.

> If that were done, would the third change still be
> needed?  (It did seem to add some other features as well, so I'm going to
> assume we still want those...).

It's still useful, yes.  It avoids flushing the WAL too often
(page-by-page when there's a lot of writes), which can eat up a lot of
IOPS on fast storage.

> But now the first change is even worse than useless, it is positively
> harmful.  The only thing to stop it from waking the WALWriter for every
> async commit is this line:
>         /* if we have already flushed that far, we're done */
>         if (WriteRqstPtr <= LogwrtResult.Flush)
>             return;
> Since LogwrtResult.Flush doesn't advance anymore, this condition doesn't
> becomes false due to us waking walwriter, it only becomes false once the
> timeout expires (which it would have done anyway with no help from us), or
> once wal_writer_flush_after is exceeded.  So now every async commit is
> waking the walwriter.  Most of those wake up do nothing (there is not a
> completely new patch to write), some write one completed page but don't
> flush anything, and very few do a flush, and that one would have been done
> anyway.

s/completely new patch/completely new page/?

In my opinion we actually *do* want to write (but not flush!) out such
pages, so I'm not sure I agree with that logic.  Have to think about
this some more...


Andres Freund
>From 50a3e62b16b752837f5a388e3259fee2bca6c2ab Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sat, 24 Jun 2017 16:46:11 -0700
Subject: [PATCH 1/3] Don't signal latch owners if owner not waiting on latch.

Doing so can generate a lot of useless syscalls and context switches
in cases where the owner of the latch is busy doing actual work.

Discussion: https://postgr.es/m/CAMkU=1z6_k4me12vnirgv5qnu58+czqdnxd+pb5bzxnvovv...@mail.gmail.com
 src/backend/storage/ipc/latch.c | 62 ++++++++++++++++++++++++++++++++++-------
 src/include/storage/latch.h     |  1 +
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 07b1364de8..c1a1a76d0a 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -221,6 +221,7 @@ InitLatch(volatile Latch *latch)
 	latch->is_set = false;
 	latch->owner_pid = MyProcPid;
+	latch->owner_waiting = false;
 	latch->is_shared = false;
 #ifndef WIN32
@@ -268,6 +269,7 @@ InitSharedLatch(volatile Latch *latch)
 	latch->is_set = false;
 	latch->owner_pid = 0;
+	latch->owner_waiting = false;
 	latch->is_shared = true;
@@ -311,6 +313,7 @@ DisownLatch(volatile Latch *latch)
 	Assert(latch->owner_pid == MyProcPid);
 	latch->owner_pid = 0;
+	latch->owner_waiting = false; /* paranoia */
@@ -466,7 +469,17 @@ SetLatch(volatile Latch *latch)
-		kill(owner_pid, SIGUSR1);
+	{
+		/*
+		 * If the owner of the latch is currently waiting on it, send signal
+		 * to wake up that process.  The read-barrier is necessary so we see
+		 * an up-to-date value, and it pairs with the memory barrier in the
+		 * path setting owner_waiting in WaitEventSetWait.
+		 */
+		pg_read_barrier();
+		if (latch->owner_waiting)
+			kill(owner_pid, SIGUSR1);
+	}
@@ -954,6 +967,12 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 		 * immediately, avoid blocking again. We don't attempt to report any
 		 * other events that might also be satisfied.
+		 * To avoid other processes signalling us if we're not waiting,
+		 * wasting context switches, we first check whether the latch is
+		 * already set, and only enable signalling from other processes after
+		 * that.  To avoid a race, we've to recheck whether the latch is set
+		 * after asking to be woken up.
+		 *
 		 * If someone sets the latch between this and the
 		 * WaitEventSetWaitBlock() below, the setter will write a byte to the
 		 * pipe (or signal us and the signal handler will do that), and the
@@ -976,17 +995,37 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 		 * ordering, so that we cannot miss seeing is_set if a notification
 		 * has already been queued.
-		if (set->latch && set->latch->is_set)
+		if (set->latch)
-			occurred_events->fd = PGINVALID_SOCKET;
-			occurred_events->pos = set->latch_pos;
-			occurred_events->user_data =
-				set->events[set->latch_pos].user_data;
-			occurred_events->events = WL_LATCH_SET;
-			occurred_events++;
-			returned_events++;
+			bool latch_set = false;
+			if (set->latch->is_set)
+			{
+				latch_set = true;
+			}
+			else
+			{
+				set->latch->owner_waiting = true;
+				pg_memory_barrier();
+				if (set->latch->is_set)
+				{
+					latch_set = true;
+				}
+			}
+			if (latch_set)
+			{
+				occurred_events->fd = PGINVALID_SOCKET;
+				occurred_events->pos = set->latch_pos;
+				occurred_events->user_data =
+					set->events[set->latch_pos].user_data;
+				occurred_events->events = WL_LATCH_SET;
+				occurred_events++;
+				returned_events++;
+				break;
+			}
-			break;
@@ -1016,6 +1055,9 @@ WaitEventSetWait(WaitEventSet *set, long timeout,
 	waiting = false;
+	if (set->latch)
+		set->latch->owner_waiting = false;
 	return returned_events;
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 73abfafec5..442344d914 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -112,6 +112,7 @@ typedef struct Latch
 	sig_atomic_t is_set;
 	bool		is_shared;
 	int			owner_pid;
+	int			owner_waiting; /* int, so all platforms can set atomically */
 #ifdef WIN32
 	HANDLE		event;

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to