Re: Logical replication timeout problem

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>

To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>

Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, Ashutosh Bapat <ashutosh.bapat.oss@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-01-24T00:28:28Z

Lists: pgsql-hackers

Hi Hou-san, Here are my review comments for v5-0001.

======
src/backend/replication/logical/reorderbuffer.c

1.
@@ -2446,6 +2452,23 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
  elog(ERROR, "tuplecid value in changequeue");
  break;
  }
+
+ /*
+ * Sending keepalive messages after every change has some overhead, but
+ * testing showed there is no noticeable overhead if keepalive is only
+ * sent after every ~100 changes.
+ */
+#define CHANGES_THRESHOLD 100
+
+ /*
+ * Try to send a keepalive message after every CHANGES_THRESHOLD
+ * changes.
+ */
+ if (++changes_count >= CHANGES_THRESHOLD)
+ {
+ rb->update_progress_txn(rb, txn, change);
+ changes_count = 0;
+ }

I noticed you put the #define adjacent to the only usage of it,
instead of with the other variable declaration like it was before.
Probably it is better how you have done it, but:

1a.
The comment indentation is incorrect.

~

1b.
Since the #define is adjacent to its only usage IMO now the 2nd
comment is redundant. So the code can just say

           /*
            * Sending keepalive messages after every change has some
overhead, but
            * testing showed there is no noticeable overhead if
keepalive is only
            * sent after every ~100 changes.
            */
#define CHANGES_THRESHOLD 100
            if (++changes_count >= CHANGES_THRESHOLD)
            {
                rb->update_progress_txn(rb, txn, change);
                changes_count = 0;
            }

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Commits

Fix the logical replication timeout during large DDLs.
- 8c58624df462 16.0 landed
Fix the logical replication timeout during large transactions.
- f95d53eded55 15.0 landed
- d6da71fa8f28 14.4 landed
- 55558df23741 13.8 landed
- f832b5007c1c 12.12 landed
- 87c1dd246af8 11.17 landed
- a4015ec0375d 10.22 landed
Rethink the delay-checkpoint-end mechanism in the back-branches.
- 10520f434687 14.3 cited
Revert "Logical decoding of sequences"
- 2c7ea57e56ca 15.0 cited
Skip empty transactions for logical replication.
- d5a9d86d8ffc 15.0 cited
Allow specifying column lists for logical replication
- 923def9a533a 15.0 cited
Add decoding of sequences to built-in replication
- 75b1521dae1f 15.0 cited
Fix ABI break introduced by commit 4daa140a2f.
- 56e366f6757d 13.4 cited
Lag tracking for logical replication
- 024711bb5446 10.0 cited