Hoi hackers,
We've been having issues with NOTIFYs blocking over multiple databases
(see [1] for more details). That was 9.4 but we've updated the
database to 11.3 and still have the same issue. Now however we could
use perf to do profiling and got the following profile (useless
details elided):
--32.83%--ProcessClientReadInterrupt
--32.68%--ProcessNotifyInterrupt
--32.16%--asyncQueueReadAllNotifications
--23.37%--asyncQueueAdvanceTail
--20.49%--LWLockAcquire
--18.93%--LWLockQueueSelf
--12.99%--LWLockWaitListLock
(from: perf record -F 99 -ag -- sleep 600)
That shows that more than 20% of the time is spent in that single
function, waiting for an exclusive lock on the AsyncQueueLock. This
will block any concurrent session doing a NOTIFY in any database on
the system. This would certainly explain the symptoms we're seeing
(process xxx still waiting for AccessExclusiveLock on object 0 of
class 1262 of database 0).
Analysis of the code leads me to the following hypothesis (and hence
to the attached patches):
We have ~150 databases, each of which has 2 active backends with an
active LISTEN. When a NOTIFY happens anywhere on any database it
(under an exclusive lock) makes a list of 300 backends to send a
signal to. It then wakes up all of those backends. Each backend then
examines the message and all but one discards it as being for the
wrong database. Each backend then calls asyncQueueAdvanceTail (because
the current position of the each backend was the tail) which then
takes an exclusive lock and checks all the other backends to see if
the tail can be advanced. All of these will conclude 'no', except the
very last one which concludes the tail can be advanced by about 50
bytes or so.
So the inner loop of asyncQueueAdvanceTail will, while holding a
global exclusive lock, execute 2*150*4000 (max backends) = 1.2 million
times for basically no benefit. During this time, no other transaction
anywhere in the system that does a NOTIFY will be able to commit.
The attached patches attempt reduce the overhead in two ways:
Patch 1: Changes asyncQueueAdvanceTail to do nothing unless the
QUEUE_HEAD is on a different page than the QUEUE_TAIL. The idea is
that there's no point trying to advance the tail unless we can
actually usefully truncate the SLRU. This does however mean that
asyncQueueReadAllNotifications always has to call
asyncQueueAdvanceTail since it can no longer be guaranteed that any
backend is still at the tail, which is one of the assumptions of the
current code. Not sure if this is a problem or if it can be improved
without tracking much more state.
Patch 2: Changes SignalBackends to only notify other backends when (a)
they're the same database as me or (b) the notify queue has advanced
to a new SLRU page. This avoids backends being woken up for messages
which they are not interested in.
As a consequence of these changes, we can reduce the number of
exclusive locks and backend wake ups in our case by a factor of 300.
You still however get a thundering herd at the end of each SLRU page.
Note: these patches have not yet been extensively tested, and so
should only be used as basis for discussion.
Comments? Suggestions?
[1]
https://www.postgresql.org/message-id/cadwg95t0j9zf0uwdcmh81kmndsitavhxmbvgyqrrjcd-ilw...@mail.gmail.com
--
Martijn van Oosterhout <klep...@gmail.com> http://svana.org/kleptog/
From 17c241a2e307e70465c235248c17ac70d34fd175 Mon Sep 17 00:00:00 2001
From: Martijn van Oosterhout <oosterh...@fox-it.com>
Date: Mon, 3 Jun 2019 17:13:31 +0200
Subject: [PATCH 1/2] Only try advancing tail pointer when it's useful.
Advancing the tail pointer requires an exclusive lock which can block
backends from other databases, so it's worth keeping these attempts to a
minimum.
---
src/backend/commands/async.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 738e6ec7e2..c1b0705234 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -89,11 +89,9 @@
* Inbound-notify processing consists of reading all of the notifications
* that have arrived since scanning last time. We read every notification
* until we reach either a notification from an uncommitted transaction or
- * the head pointer's position. Then we check if we were the laziest
- * backend: if our pointer is set to the same position as the global tail
- * pointer is set, then we move the global tail pointer ahead to where the
- * second-laziest backend is (in general, we take the MIN of the current
- * head position and all active backends' new tail pointers). Whenever we
+ * the head pointer's position. Then we check if we can advance the global
+ * tail pointer to a new page, if so we take the MIN of the current
+ * head position and all active backends' new tail pointers. Whenever we
* move the global tail pointer we also truncate now-unused pages (i.e.,
* delete files in pg_notify/ that are no longer used).
*
@@ -1749,7 +1747,6 @@ asyncQueueReadAllNotifications(void)
QueuePosition oldpos;
QueuePosition head;
Snapshot snapshot;
- bool advanceTail;
/* page_buffer must be adequately aligned, so use a union */
union
@@ -1871,12 +1868,10 @@ asyncQueueReadAllNotifications(void)
/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
QUEUE_BACKEND_POS(MyBackendId) = pos;
- advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);
- /* If we were the laziest backend, try to advance the tail pointer */
- if (advanceTail)
- asyncQueueAdvanceTail();
+ /* Check if tail can be advanced */
+ asyncQueueAdvanceTail();
PG_RE_THROW();
}
@@ -1885,12 +1880,10 @@ asyncQueueReadAllNotifications(void)
/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
QUEUE_BACKEND_POS(MyBackendId) = pos;
- advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);
- /* If we were the laziest backend, try to advance the tail pointer */
- if (advanceTail)
- asyncQueueAdvanceTail();
+ /* Check if tail can be advanced */
+ asyncQueueAdvanceTail();
/* Done with snapshot */
UnregisterSnapshot(snapshot);
@@ -2010,6 +2003,14 @@ asyncQueueAdvanceTail(void)
int newtailpage;
int boundary;
+ /*
+ * Advancing the tail is expensive (it takes an exclusive lock which
+ * can block committing backends) so don't bother if we can't advance
+ * at least a page.
+ */
+ if (QUEUE_POS_PAGE(QUEUE_TAIL) != QUEUE_POS_PAGE(QUEUE_HEAD))
+ return;
+
LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
min = QUEUE_HEAD;
for (i = 1; i <= MaxBackends; i++)
--
2.11.0
From bfefa25b210993450d89381ad26b5b77a37091bc Mon Sep 17 00:00:00 2001
From: Martijn van Oosterhout <oosterh...@fox-it.com>
Date: Mon, 3 Jun 2019 17:10:38 +0200
Subject: [PATCH 2/2] Don't notify other backends about notifications without
cause.
It could be that there are other databases with active listens but unless
they need to know because it may be useful to advance the queue tail,
there's no point waking them up.
---
src/backend/commands/async.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index c1b0705234..bb4f58d0b9 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -1519,11 +1519,13 @@ SignalBackends(void)
int count;
int i;
int32 pid;
+ int notify_all = false;
/*
* Identify all backends that are listening and not already up-to-date. We
* don't want to send signals while holding the AsyncQueueLock, so we just
- * build a list of target PIDs.
+ * build a list of target PIDs. If we haven't moved to a new page there is
+ * no point notifying backends of other databases.
*
* XXX in principle these pallocs could fail, which would be bad. Maybe
* preallocate the arrays? But in practice this is only run in trivial
@@ -1532,6 +1534,7 @@ SignalBackends(void)
pids = (int32 *) palloc(MaxBackends * sizeof(int32));
ids = (BackendId *) palloc(MaxBackends * sizeof(BackendId));
count = 0;
+ notify_all = QUEUE_POS_PAGE(QUEUE_HEAD) != QUEUE_POS_PAGE(QUEUE_TAIL);
LWLockAcquire(AsyncQueueLock, LW_EXCLUSIVE);
for (i = 1; i <= MaxBackends; i++)
@@ -1539,6 +1542,9 @@ SignalBackends(void)
pid = QUEUE_BACKEND_PID(i);
if (pid != InvalidPid && pid != MyProcPid)
{
+ if (!notify_all && QUEUE_BACKEND_DBOID(i) != MyDatabaseId)
+ continue;
+
QueuePosition pos = QUEUE_BACKEND_POS(i);
if (!QUEUE_POS_EQUAL(pos, QUEUE_HEAD))
--
2.11.0
