Message-ID: <2d314c22b9e03415aa1c7d8fd1f698dae60effa7.camel@j-davis.com>
Subject: Re: Minimal logical decoding on standbys
From: Jeff Davis <pgsql@j-davis.com>
To: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>, Andres Freund
	 <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Thomas Munro
 <thomas.munro@gmail.com>,  Alvaro Herrera <alvherre@2ndquadrant.com>, Ibrar
 Ahmed <ibrar.ahmad@gmail.com>, Amit Khandekar <amitdkhan.pg@gmail.com>,
 fabriziomello@gmail.com, tushar <tushar.ahuja@enterprisedb.com>, Rahila
 Syed <rahila.syed@2ndquadrant.com>,  pgsql-hackers
 <pgsql-hackers@postgresql.org>
Date: Tue, 28 Feb 2023 16:48:44 -0800
In-Reply-To: <a4c03fad-2986-ddab-c12e-ea2ee0119181@gmail.com>
References: 
	<CA+TgmoZ8-DXtDMMDGggFkSYyO8nmNdBKWjV6Ts-zYGR9p5sK5g@mail.gmail.com>
	 <CA+TgmobmTvEUDVOBzARa9eD9XLqsvDW+v29dV+WvDKZPJAwNTQ@mail.gmail.com>
	 <CA+TgmoaOsoQuXEo6gMq0GP8SyvCaS=ptYqdYc0uzDMoeShf_nw@mail.gmail.com>
	 <CA+TgmoYTTsxP8y6uknZvCBNCRq+1FJ4zGbX8Px1TGW459fGsaQ@mail.gmail.com>
	 <1f2ad9fd-53ff-74cc-a602-72d45423bcab@gmail.com>
	 <a78de011-81cf-729c-e110-e6d636fdca4d@gmail.com>
	 <05c0c4f3-82eb-bc6d-c876-5a265c05927b@gmail.com>
	 <CA+TgmoY8LAaTwAhS+YNQNrM+3K8eqxGvPT3gbJaHER86XShvFw@mail.gmail.com>
	 <20230106034036.2m4qnn7ep7b5ipet@awork3.anarazel.de>
	 <490206c5-bcde-e558-628a-dc67dae4d439@gmail.com>
	 <20230119024617.jzsgpcrgjdc6ysge@awork3.anarazel.de>
	 <a343b646-c632-4b9a-0e01-b661075d1d8b@gmail.com>
	 <b74520d5-5b9e-1477-2abc-95ba897f23e4@gmail.com>
	 <94d9d47e-72f4-393a-246e-e3fe8db7b741@gmail.com>
	 <a4c03fad-2986-ddab-c12e-ea2ee0119181@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/2d314c22b9e03415aa1c7d8fd1f698dae60effa7.camel%40j-davis.com>
Precedence: bulk

On Mon, 2023-02-27 at 09:40 +0100, Drouvot, Bertrand wrote:
> Please find attached V51 tiny rebase due to a6cd1fc692 (for 0001) and
> 8a8661828a (for 0005).

[ Jumping into this thread late, so I apologize if these comments have
already been covered. ]

Regarding v51-0004:

* Why is the CV sleep not being canceled?
* Comments on WalSndWaitForWal need to be updated to explain the
difference between the flush (primary) and the replay (standby) cases.

Overall, it seems like what you really want for the sleep/wakeup logic
in WalSndWaitForLSN is something like this:

   condVar =3D RecoveryInProgress() ? replayCV : flushCV;
   waitEvent =3D RecoveryInProgress() ?=20
       WAIT_EVENT_WAL_SENDER_WAIT_REPLAY :
       WAIT_EVENT_WAL_SENDER_WAIT_FLUSH;

   ConditionVariablePrepareToSleep(condVar);
   for(;;)
   {
      ...
      sleeptime =3D WalSndComputeSleepTime(GetCurrentTimestamp());
      socketEvents =3D WL_SOCKET_READABLE;
      if (pq_is_send_pending())
          socketEvents =3D WL_SOCKET_WRITABLE;
      ConditionVariableTimedSleepOrEvents(
          condVar, sleeptime, socketEvents, waitEvent);
   }
   ConditionVariableCancelSleep();


But the problem is that ConditionVariableTimedSleepOrEvents() doesn't
exist, and I think that's what Andres was suggesting here[1].
WalSndWait() only waits for a timeout or a socket event, but not a CV;
ConditionVariableTimedSleep() only waits for a timeout or a CV, but not
a socket event.

I'm also missing how WalSndWait() works currently. It calls
ModifyWaitEvent() with NULL for the latch, so how does WalSndWakeup()
wake it up?

Assuming I'm wrong, and WalSndWait() does use the latch, then I guess
it could be extended by having two different latches in the WalSnd
structure, and waking them up separately and waiting on the right one.
Not sure if that's a good idea though.

[1]
https://www.postgresql.org/message-id/20230106034036.2m4qnn7ep7b5ipet@awork=
3.anarazel.de

--=20
Jeff Davis
PostgreSQL Contributor Team - AWS