Horton

RE: Logical replication timeout problem

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread View on postgr.es
From: "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>
To: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>
Cc: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>, Simon Riggs <simon.riggs@enterprisedb.com>, Petr Jelinek <petr.jelinek@enterprisedb.com>, Fabrice Chapuis <fabrice636861@gmail.com>
Date: 2022-02-24T08:06:29Z
Lists: pgsql-hackers 
Dear Wang,

Thank you for teaching some backgrounds about the patch.

> According to our discussion, we need to send keepalive messages to subscriber
> when skipping changes.
> One approach is that **for each skipped change**, we try to send keepalive
> message by calculating whether a timeout will occur based on the current time
> and the last time the keepalive was sent. But this will brings slight overhead.
> So I want to try another approach: after **constantly skipping some changes**,
> we try to send keepalive message by calculating whether a timeout will occur
> based on the current time and the last time the keepalive was sent.

You meant that calling system calls like GetCurrentTimestamp() should be reduced,
right? I'm not sure how it affects but it seems reasonable.

> IMO, we should send keepalive message after skipping a certain number of
> changes constantly.
> And I want to calculate the threshold dynamically by using a fixed value to
> avoid adding too much code.
> In addition, different users have different machine performance, and users can
> modify wal_sender_timeout, so the threshold should be dynamically calculated
> according to wal_sender_timeout.

Your experiment seems not bad, but the background cannot be understand from
code comments. I prefer a static threshold because it's more simple,
which as Amit said in the following too:

https://www.postgresql.org/message-id/CAA4eK1%2B-p_K_j%3DNiGGD6tCYXiJH0ypT4REX5PBKJ4AcUoF3gZQ%40mail.gmail.com

> In the existing code, similar operations on wal_sender_timeout use the style of
> (wal_sender_timeout / 2), e.g. function WalSndKeepaliveIfNecessary. So I think
> it should be consistent in this patch.
> But I think it is better to use magic number too. Maybe we could improve it in
> a new thread.

I confirmed the code and +1 yours. We should treat it in another thread if needed.

BTW, this patch cannot be applied to current master.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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