Horton

Re: Logical replication timeout problem

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, Fabrice Chapuis <fabrice636861@gmail.com>, Euler Taveira <euler@eulerto.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Simon Riggs <simon.riggs@enterprisedb.com>, Petr Jelinek <petr.jelinek@enterprisedb.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Ajin Cherian <itsajin@gmail.com>
Date: 2023-02-08T05:27:52Z
Lists: pgsql-hackers 
Hi,

On 2023-02-03 10:13:54 +0530, Amit Kapila wrote:
> I am planning to push this to HEAD sometime next week (by Wednesday).
> To backpatch this, we need to fix it in some non-standard way, like
> without introducing a callback which I am not sure is a good idea. If
> some other committers vote to get this in back branches with that or
> some different idea that can be backpatched then we can do that
> separately as well. I don't see this as a must-fix in back branches
> because we have a workaround (increase timeout) or users can use the
> streaming option (for >=14).

I just saw the commit go in, and a quick scan over it makes me think neither
this commit, nor f95d53eded, which unfortunately was already backpatched, is
the right direction. The wrong direction likely started quite a bit earlier,
with 024711bb544.

It feels quite fundamentally wrong that bascially every output plugin needs to
call a special function in nearly every callback.

In 024711bb544 there was just one call to OutputPluginUpdateProgress() in
pgoutput.c. Quite tellingly, it just updated pgoutput, without touching
test_decoding.

Then a8fd13cab0b added to more calls. 63cf61cdeb7 yet another.


This makes no sense.  There's lots of output plugins out there. There's an
increasing number of callbacks.  This isn't a maintainable path forward.


If we want to call something to maintain state, it has to be happening from
central infrastructure.


It feels quite odd architecturally that WalSndUpdateProgress() ends up
flushing out writes - that's far far from obvious.

I don't think:
/*
 * Wait until there is no pending write. Also process replies from the other
 * side and check timeouts during that.
 */
static void
ProcessPendingWrites(void)

Is really a good name. What are we processing? What are we actually waiting
for - because we don't actually wait for the data to sent out or anything,
just that they're in a network buffer.


Greetings,

Andres Freund

Commits

  1. Fix the logical replication timeout during large DDLs.

  2. Fix the logical replication timeout during large transactions.

  3. Rethink the delay-checkpoint-end mechanism in the back-branches.

  4. Revert "Logical decoding of sequences"

  5. Skip empty transactions for logical replication.

  6. Allow specifying column lists for logical replication

  7. Add decoding of sequences to built-in replication

  8. Fix ABI break introduced by commit 4daa140a2f.

  9. Lag tracking for logical replication