MIME-Version: 1.0
References: <20230509190247.3rrplhdgem6su6cg@awork3.anarazel.de>
 <CALj2ACVZyh++dgq5REgmO5-wNtZt=tN4zv=kMqPEARJ4zOpfVg@mail.gmail.com>
 <OS0PR01MB5716CADAD77D0D720E27607A94779@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LTcqyaSBdACRM+QyZtDO6zd-40NoqMoxRbW4TQ5yAtUA@mail.gmail.com>
In-Reply-To: <CAA4eK1LTcqyaSBdACRM+QyZtDO6zd-40NoqMoxRbW4TQ5yAtUA@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 11 May 2023 13:56:52 +0900
Message-ID: <CAD21AoC5OnSc9BeYRLFQdP0PpHoT7z30DpAKxrb2P_P+fO-rZA@mail.gmail.com>
Subject: Re: walsender performance regression due to logical decoding on
 standby changes
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, 
	"Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>, Jeff Davis <pgsql@j-davis.com>, 
	Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoC5OnSc9BeYRLFQdP0PpHoT7z30DpAKxrb2P_P%2BfO-rZA%40mail.gmail.com>
Precedence: bulk

On Wed, May 10, 2023 at 7:33=E2=80=AFPM Amit Kapila <amit.kapila16@gmail.co=
m> wrote:
>
> On Wed, May 10, 2023 at 3:41=E2=80=AFPM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wednesday, May 10, 2023 2:36 PM Bharath Rupireddy <bharath.rupireddy=
forpostgres@gmail.com> wrote:
> > >
> > >
> > > > My current guess is that mis-using a condition variable is the best=
 bet. I
> > > > think it should work to use ConditionVariablePrepareToSleep() befor=
e a
> > > > WalSndWait(), and then ConditionVariableCancelSleep(). I.e. to neve=
r use
> > > > ConditionVariableSleep(). The latch set from ConditionVariableBroad=
cast()
> > > > would still cause the necessary wakeup.
> > >
> > > How about something like the attached? Recovery and subscription test=
s
> > > don't complain with the patch.
> >
> > Thanks for the patch. I noticed one place where the logic is different =
from before and
> > just to confirm:
> >
> >         if (AllowCascadeReplication())
> > -               WalSndWakeup(switchedTLI, true);
> > +               ConditionVariableBroadcast(&WalSndCtl->cv);
> >
> > After the change, we wakeup physical walsender regardless of switchedTL=
I flag.
> > Is this intentional ? if so, I think It would be better to update the c=
omments above this.
> >
>
> This raises the question of whether we need this condition variable
> logic for physical walsenders?

It sounds like a good idea. We can have two condition variables for
logical and physical walsenders, and selectively wake up walsenders
sleeping on the condition variables. It should work, it seems like
much of a hack, though.

Regards,

[1] https://www.postgresql.org/message-id/2d314c22b9e03415aa1c7d8fd1f698dae=
60effa7.camel%40j-davis.com

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