public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bharath Rupireddy <[email protected]>
To: Andres Freund <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Drouvot, Bertrand <[email protected]>
Cc: [email protected]
Cc: Jeff Davis <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: walsender performance regression due to logical decoding on standby changes
Date: Thu, 18 May 2023 20:11:11 +0530
Message-ID: <CALj2ACXtHuS9PcP+R2TiMdSx_GepYSfx8n1O0pZSAPZQY2PWJA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CALj2ACVZyh++dgq5REgmO5-wNtZt=tN4zv=kMqPEARJ4zOpfVg@mail.gmail.com>
<[email protected]>
<CALj2ACVDj+D-nPkzu0f06fMwigBJHwx03FEpfsz427G6AasKWA@mail.gmail.com>
<CA+hUKG+iywOmdiGMOegUXuVDQzhgpSx9c5vpzOvYUEVDu_gZBQ@mail.gmail.com>
<CAD21AoCv+9KyQ0BCNnd5CH53GNshSgGzvzt1JQkhueh8XnSi7A@mail.gmail.com>
<CALj2ACWeo64RSqf8tDbnSSUm_vbpK5GYdDiiFQk8E3Fg38mBdw@mail.gmail.com>
<[email protected]>
On Thu, May 18, 2023 at 1:23 AM Andres Freund <[email protected]> wrote:
>
> On 2023-05-15 20:09:00 +0530, Bharath Rupireddy wrote:
> > > [1]
> > > max_wal_senders = 100
> > > before regression(ms) after regression(ms) v2 patch(ms)
> > > 13394.4013 14141.2615 13455.2543
> > > Compared with before regression 5.58% 0.45%
> > >
> > > max_wal_senders = 200
> > > before regression(ms) after regression(ms) v2 patch(ms)
> > > 13280.8507 14597.1173 13632.0606
> > > Compared with before regression 9.91% 1.64%
> > >
> > > max_wal_senders = 300
> > > before regression(ms) after regression(ms) v2 patch(ms)
> > > 13535.0232 16735.7379 13705.7135
> > > Compared with before regression 23.65% 1.26%
> >
> > Yes, the numbers with v2 patch look close to where we were before.
> > Thanks for confirming. Just wondering, where does this extra
> > 0.45%/1.64%/1.26% coming from?
>
> We still do more work for each WAL record than before, so I'd expect something
> small. I'd say right now the main overhead with the patch comes from the
> spinlock acquisitions in ConditionVariableBroadcast(), which happen even when
> nobody is waiting.
> > + ConditionVariableInit(&WalSndCtl->physicalWALSndCV);
> > + ConditionVariableInit(&WalSndCtl->logicalWALSndCV);
>
> It's not obvious to me that it's worth having two CVs, because it's more
> expensive to find no waiters in two CVs than to find no waiters in one CV.
I disagree. In the tight per-WAL record recovery loop, WalSndWakeup
wakes up logical walsenders for every WAL record, but it wakes up
physical walsenders only if the applied WAL record causes a TLI
switch. Therefore, the extra cost of spinlock acquire-release for per
WAL record applies only for logical walsenders. On the other hand, if
we were to use a single CV, we would be unnecessarily waking up (if at
all they are sleeping) physical walsenders for every WAL record -
which is costly IMO.
I still think separate CVs are good for selective wake ups given there
can be not so many TLI switch WAL records.
> > + *
> > + * XXX: When available, WaitEventSetWait() can be replaced with its CV's
> > + * counterpart.
>
> I don't really understand that XXX - the potential bright future would be to
> add support for CVs into wait event sets, not to replace WES with a CV?
Yes, I meant it and modified that part to:
* XXX: A desirable future improvement would be to add support for CVs into
* WaitEventSetWait().
> FWIW, if I just make WalSndWakeup() do nothing, I still see a very small, but
> reproducible, overhead: 1.72s - that's just the cost of the additional
> external function call.
Hm, this is unavoidable.
> If I add a no-waiters fastpath using proclist_is_empty() to
> ConditionVariableBroadcast(), I get 1.77s. So the majority of the remaining
> slowdown indeed comes from the spinlock acquisition in
> ConditionVariableBroadcast().
Acquiring spinlock for every replayed WAL record seems not great. In
general, this problem exists for CV infrastructure as a whole if
ConditionVariableBroadcast()/CVB() is called in a loop/hot path. I
think this can be proven easily by doing something like - select
pg_replication_origin_session_setup()/pg_wal_replay_resume()/pg_create_physical_replication_slot()
from generate_series(1, 1000000...);, all of these functions end up in
CVB().
Do you think adding a fastpath exit when no waiters to CVB() is worth
implementing? Something like - an atomic state variable to each CV,
similar to LWLock's state variable, setting it when adding waiters and
resetting it when removing waiters, CVB() atomically reading the state
for fastpath (of course, we need memory barriers here). This might
complicate things as each CV structure gets an extra state variable (4
bytes), memory barriers, and atomic writes and reads.
Or given the current uses of CVB() with no callers except recovery
calling it in a hot path with patch, maybe we can add an atomic waiter
count to WalSndCtl, incrementing it atomically in WalSndWait() before
the wait, decrementing it after the wait, and a fastpath in
WalSndWakeup() reading it atomically to avoid CVB() calls. IIUC, this
is something Jeff proposed upthread.
Thoughts?
Please find the attached v4 patch addressing the review comment (not
the fastpath one).
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v4-0001-Optimize-walsender-wake-up-logic-with-Conditional.patch (6.3K, ../CALj2ACXtHuS9PcP+R2TiMdSx_GepYSfx8n1O0pZSAPZQY2PWJA@mail.gmail.com/2-v4-0001-Optimize-walsender-wake-up-logic-with-Conditional.patch)
download | inline diff:
From 00234c83cedcb684f4b6e12d22608f929f4d3c0b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 18 May 2023 14:15:05 +0000
Subject: [PATCH v4] Optimize walsender wake up logic with Conditional
Variables
WalSndWakeup() currently loops through all the walsenders slots,
with a spinlock acquisition and release for every iteration, just
to wake up only the waiting walsenders. WalSndWakeup() gets
costlier even when there's a single walsender available on the
standby (i.e., a cascading replica or a logical decoding client).
This wasn't a problem before e101dfac3a53c. But, for allowing
logical decoding on standby, we needed to wake up logical
walsenders after every WAL record is applied on the standby. This
really made WalSndWakeup() costly, causing performance regression.
To solve this, we use condition variable (CV) to efficiently wake
up walsenders in WalSndWakeup().
Every walsender prepares to sleep on a shared memory CV. Note that
it just prepares to sleep on the CV (i.e., adds itself to the CV's
waitlist), but does not actually wait on the CV (IOW, it never
calls ConditionVariableSleep()). It still uses WaitEventSetWait()
for waiting, because CV infrastructure doesn't handle FeBe socket
events currently. The processes (startup process, walreceiver etc.)
wanting to wake up walsenders use ConditionVariableBroadcast(),
which in turn calls SetLatch(), helping walsenders come out of
WaitEventSetWait().
This approach is simple and efficient because it makes
WalSndWakeup() life easy. A desirable future improvement would be to
add support for CVs into WaitEventSetWait().
And, we use separate shared memory CVs for physical and logical
walsenders for selective wake ups, see WalSndWakeup() for more
details.
Reported-by: Andres Freund
Suggested-by: Andres Freund
Author: Bharath Rupireddy
Reviewed-by: Drouvot, Bertrand
Reviewed-by: Zhijie Hou
Discussion: https://www.postgresql.org/message-id/20230509190247.3rrplhdgem6su6cg%40awork3.anarazel.de
---
src/backend/replication/walsender.c | 72 ++++++++++++++-------
src/include/replication/walsender_private.h | 7 ++
2 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 45b8b3684f..57ef3dbb67 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -3309,6 +3309,9 @@ WalSndShmemInit(void)
SpinLockInit(&walsnd->mutex);
}
+
+ ConditionVariableInit(&WalSndCtl->physicalWALSndCV);
+ ConditionVariableInit(&WalSndCtl->logicalWALSndCV);
}
}
@@ -3330,31 +3333,17 @@ WalSndShmemInit(void)
void
WalSndWakeup(bool physical, bool logical)
{
- int i;
-
- for (i = 0; i < max_wal_senders; i++)
- {
- Latch *latch;
- ReplicationKind kind;
- WalSnd *walsnd = &WalSndCtl->walsnds[i];
-
- /*
- * Get latch pointer with spinlock held, for the unlikely case that
- * pointer reads aren't atomic (as they're 8 bytes). While at it, also
- * get kind.
- */
- SpinLockAcquire(&walsnd->mutex);
- latch = walsnd->latch;
- kind = walsnd->kind;
- SpinLockRelease(&walsnd->mutex);
-
- if (latch == NULL)
- continue;
+ /*
+ * Let's wake up all the waiting physical and/or logical walsenders using
+ * their respective condition variables (CVs). Note that every walsender
+ * would have prepared to sleep on the CV (i.e., added itself to the CV's
+ * waitlist) in WalSndWait before actually waiting.
+ */
+ if (physical)
+ ConditionVariableBroadcast(&WalSndCtl->physicalWALSndCV);
- if ((physical && kind == REPLICATION_KIND_PHYSICAL) ||
- (logical && kind == REPLICATION_KIND_LOGICAL))
- SetLatch(latch);
- }
+ if (logical)
+ ConditionVariableBroadcast(&WalSndCtl->logicalWALSndCV);
}
/*
@@ -3368,9 +3357,44 @@ WalSndWait(uint32 socket_events, long timeout, uint32 wait_event)
WaitEvent event;
ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, socket_events, NULL);
+
+ /*
+ * We use condition variable (CV) to efficiently wake up walsenders in
+ * WalSndWakeup().
+ *
+ * Every walsender prepares to sleep on a shared memory CV. Note that it
+ * just prepares to sleep on the CV (i.e., adds itself to the CV's
+ * waitlist), but does not actually wait on the CV (IOW, it never calls
+ * ConditionVariableSleep()). It still uses WaitEventSetWait() for waiting,
+ * because CV infrastructure doesn't handle FeBe socket events currently.
+ * The processes (startup process, walreceiver etc.) wanting to wake up
+ * walsenders use ConditionVariableBroadcast(), which in turn calls
+ * SetLatch(), helping walsenders come out of WaitEventSetWait().
+ *
+ * This approach is simple and efficient because, one doesn't have to loop
+ * through all the walsenders slots, with a spinlock acquisition and
+ * release for every iteration, just to wake up only the waiting
+ * walsenders. It makes WalSndWakeup() callers' life easy.
+ *
+ * XXX: A desirable future improvement would be to add support for CVs into
+ * WaitEventSetWait().
+ *
+ * And, we use separate shared memory CVs for physical and logical
+ * walsenders for selective wake ups, see WalSndWakeup() for more details.
+ */
+ if (MyWalSnd->kind == REPLICATION_KIND_PHYSICAL)
+ ConditionVariablePrepareToSleep(&WalSndCtl->physicalWALSndCV);
+ else if (MyWalSnd->kind == REPLICATION_KIND_LOGICAL)
+ ConditionVariablePrepareToSleep(&WalSndCtl->logicalWALSndCV);
+
if (WaitEventSetWait(FeBeWaitSet, timeout, &event, 1, wait_event) == 1 &&
(event.events & WL_POSTMASTER_DEATH))
+ {
+ ConditionVariableCancelSleep();
proc_exit(1);
+ }
+
+ ConditionVariableCancelSleep();
}
/*
diff --git a/src/include/replication/walsender_private.h b/src/include/replication/walsender_private.h
index ff25aa70a8..4b33a96d25 100644
--- a/src/include/replication/walsender_private.h
+++ b/src/include/replication/walsender_private.h
@@ -17,6 +17,7 @@
#include "nodes/nodes.h"
#include "nodes/replnodes.h"
#include "replication/syncrep.h"
+#include "storage/condition_variable.h"
#include "storage/latch.h"
#include "storage/shmem.h"
#include "storage/spin.h"
@@ -108,6 +109,12 @@ typedef struct
*/
bool sync_standbys_defined;
+ /* Used to wake up physical walsenders */
+ ConditionVariable physicalWALSndCV;
+
+ /* Used to wake up logical walsenders */
+ ConditionVariable logicalWALSndCV;
+
WalSnd walsnds[FLEXIBLE_ARRAY_MEMBER];
} WalSndCtlData;
--
2.34.1
view thread (17+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: walsender performance regression due to logical decoding on standby changes
In-Reply-To: <CALj2ACXtHuS9PcP+R2TiMdSx_GepYSfx8n1O0pZSAPZQY2PWJA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox