Subject: Re: walsender performance regression due to logical decoding on standby changes

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 19 May 2023 12:07:56 +0900 (JST)

At Thu, 18 May 2023 20:11:11 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> > > +             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.

As I was reading this, I start thinking that one reason for the
regression could be to exccessive frequency of wakeups during logical
replication. In physical replication, we make sure to avoid exccessive
wakeups when the stream is tightly packed.  I'm just wondering why
logical replication doesn't (or can't) do the same thing, IMHO.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Andres Freund <[email-masked]>
Date: Sun, 21 May 2023 09:10:46 -0700

Hi,

> logical replication doesn't (or can't) do the same thing, IMHO.

It's possible we could try to reduce the frequency by issuing wakeups only at
specific points. The most obvious thing to do would be to wake only when
waiting for more WAL or when crossing a page boundary, or such. Unfortunately
that could easily lead to deadlocks, because the startup process might be
blocked waiting for a lock, held by a backend doing logical decoding - which
can't progress until the startup process wakes the backend up.

So I don't think this is promising avenue in the near term.

Greetings,

Andres Freund

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 22 May 2023 12:15:07 +0000

Hi,

On Monday, May 22, 2023 12:11 AM Andres Freund <[email-masked]> wrote:
> > At Thu, 18 May 2023 20:11:11 +0530, Bharath Rupireddy
> <[email-masked]> wrote in
> > > > > +
> ConditionVariableInit(&WalSndCtl->physicalWALSndCV);

Just out of curiosity about the mentioned deadlock scenario, no comment on the
committed patch.

About "a backend doing logical decoding", do you mean the case when a user
start a backend and invoke pg_logical_slot_get_changes() to do the logical
decoding ? If so, it seems the logical decoding in a backend won't be waked up
by startup process because the backend won't be registered as a walsender so
the backend won't be found in WalSndWakeup().

Or do you mean the deadlock between the real logical walsender and startup
process ? (I might miss something) I think the logical decoding doesn't lock
the target user relation when decoding because it normally can get the needed
information from WAL. Besides, the walsender sometimes will lock the system
table(e.g. use RelidByRelfilenumber() to get the relid) but it will unlock it
after finishing systable scan.

So, if possible, would you be able to share some details about the deadlock
case you mentioned earlier? it's helpful as it can prevent similar problems in
the future development.

Best Regards,
Hou zj

---

From: Andres Freund <[email-masked]>
Date: Mon, 22 May 2023 10:52:54 -0700

Hi,

> the backend won't be found in WalSndWakeup().

I meant logical decoding happening inside a walsender instance.

> information from WAL.

It does lock catalog tables briefly. There's no guarantee that such locks are
released immediately. I forgot the details, but IIRC there's some outfuncs
(enum?) that intentionally delay releasing locks till transaction commit.

Greetings,

Andres Freund

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 24 May 2023 05:53:51 +0000

> (enum?) that intentionally delay releasing locks till transaction commit.

Thanks for the explanation !

I understand that the startup process can take lock on the catalog(when
replaying record) which may conflict with the lock in walsender.

But in walsender, I think we only start transaction after entering
ReorderBufferProcessTXN(), and the transaction started here will be released
soon after processing and outputting the decoded transaction's data(as the
comment in ReorderBufferProcessTXN() says:" all locks acquired in here to be
released, not reassigned to the parent and we do not want any database access
have persistent effects.").

Besides, during the process and output of the decoded transaction, the
walsender won't wait for the wakeup of startup process(e.g.
WalSndWaitForWal()), it only waits if the data is being sent to subscriber. So
it seems the lock conflict here won't cause the deadlock for now, although it
may have a risk if we change this logic later. Sorry if I missed something, and
thanks again for your patience in explaining.

Best Regards,
Hou zj