public inbox for [email protected]  

help / color / mirror / Atom feed

From: Masahiko Sawada <[email protected]>
To: Jeff Davis <[email protected]>
Cc: Drouvot, Bertrand <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Ibrar Ahmed <[email protected]>
Cc: Amit Khandekar <[email protected]>
Cc: [email protected]
Cc: tushar <[email protected]>
Cc: Rahila Syed <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Minimal logical decoding on standbys
Date: Fri, 31 Mar 2023 01:31:41 +0900
Message-ID: <CAD21AoAyVB9YZJtYNeJnfLg9kqhbp=6thp0Y6_6Ld4=x4sKa5g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CA+TgmoZ8-DXtDMMDGggFkSYyO8nmNdBKWjV6Ts-zYGR9p5sK5g@mail.gmail.com>
	<CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
	<CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CA+TgmoY8LAaTwAhS+YNQNrM+3K8eqxGvPT3gbJaHER86XShvFw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Hi,

On Thu, Mar 30, 2023 at 2:45 PM Jeff Davis <[email protected]> wrote:
>
> On Thu, 2023-03-02 at 23:58 -0800, Jeff Davis wrote:
> > On Thu, 2023-03-02 at 11:45 -0800, Jeff Davis wrote:
> > > In this case it looks easier to add the right API than to be sure
> > > about
> > > whether it's needed or not.
> >
> > I attached a sketch of one approach. I'm not very confident that it's
> > the right API or even that it works as I intended it, but if others
> > like the approach I can work on it some more.
>
> Another approach might be to extend WaitEventSets() to be able to wait
> on Condition Variables, rather than Condition Variables waiting on
> WaitEventSets. Thoughts?
>

+1 to extend CV. If we extend WaitEventSet() to be able to wait on CV,
it would be able to make the code simple, but we would need to change
both CV and WaitEventSet().

On Fri, Mar 10, 2023 at 8:34 PM Drouvot, Bertrand
<[email protected]> wrote:
>
> I gave it a try, so please find attached v2-0001-Introduce-ConditionVariableEventSleep.txt (implementing the comments above) and 0004_new_API.txt to put the new API in the logical decoding on standby context.

@@ -180,13 +203,25 @@ ConditionVariableTimedSleep(ConditionVariable
*cv, long timeout,
                 * by something other than ConditionVariableSignal;
though we don't
                 * guarantee not to return spuriously, we'll avoid
this obvious case.
                 */
-               SpinLockAcquire(&cv->mutex);
-               if (!proclist_contains(&cv->wakeup, MyProc->pgprocno,
cvWaitLink))
+
+               if (cv)
                {
-                       done = true;
-                       proclist_push_tail(&cv->wakeup,
MyProc->pgprocno, cvWaitLink);
+                       SpinLockAcquire(&cv->mutex);
+                       if (!proclist_contains(&cv->wakeup,
MyProc->pgprocno, cvWaitLink))
+                       {
+                               done = true;
+                               proclist_push_tail(&cv->wakeup,
MyProc->pgprocno, cvWaitLink);
+                       }
+                       SpinLockRelease(&cv->mutex);
                }

This change looks odd to me since it accepts cv being NULL in spite of
calling ConditionVariableEventSleep() for cv. I think that this is
because in 0004_new_API.txt, we use ConditionVariableEventSleep() in
both not-in-recovery case and recovery-in-progress cases in
WalSndWaitForWal() as follows:

-               WalSndWait(wakeEvents, sleeptime,
WAIT_EVENT_WAL_SENDER_WAIT_WAL);
+               ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos,
wakeEvents, NULL);
+               ConditionVariableEventSleep(cv, RecoveryInProgress,
FeBeWaitSet, NULL,
+
 sleeptime, wait_event);
        }

But I don't think we need to use ConditionVariableEventSleep() in
not-in-recovery cases. If I correctly understand the problem this
patch wants to deal with, in logical decoding on standby cases, the
walsender needs to be woken up on the following events:

* condition variable
* timeout
* socket writable (if pq_is_send_pending() is true)
(socket readable event should also be included to avoid
wal_receiver_timeout BTW?)

On the other hand, in not-in-recovery case, the events are:

* socket readable
* socket writable (if pq_is_send_pending() is true)
* latch
* timeout

I think that we don't need to change for the latter case as
WalSndWait() perfectly works. As for the former cases, since we need
to wait for CV, timeout, or socket writable we can use
ConditionVariableEventSleep().

Regards,


--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

view thread (41+ messages)  latest in thread

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], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Minimal logical decoding on standbys
  In-Reply-To: <CAD21AoAyVB9YZJtYNeJnfLg9kqhbp=6thp0Y6_6Ld4=x4sKa5g@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