Horton

Re: Logical replication timeout problem

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Euler Taveira <euler@eulerto.com>, "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, Fabrice Chapuis <fabrice636861@gmail.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: 2022-04-14T12:20:13Z
Lists: pgsql-hackers 
On Wed, Apr 13, 2022 at 7:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 12:09 PM wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> >
> > So I skip tracking lag during a transaction just like the current HEAD.
> > Attach the new patch.
> >
>
> Thanks, please find the updated patch where I have slightly modified
> the comments.
>
> Sawada-San, Euler, do you have any opinion on this approach? I
> personally still prefer the approach implemented in v10 [1] especially
> due to the latest finding by Wang-San that we can't update the
> lag-tracker apart from when it is invoked at the transaction end.
> However, I am fine if we like this approach more.

Thank you for updating the patch.

The current patch looks much better than v10 which requires to call to
update_progress() every path.

Regarding v15 patch, I'm concerned a bit that the new function name,
update_progress(), is too generic. How about
update_replation_progress() or something more specific name?

---
+        if (end_xact)
+        {
+                /* Update progress tracking at xact end. */
+                OutputPluginUpdateProgress(ctx, skipped_xact, end_xact);
+                changes_count = 0;
+                return;
+        }
+
+        /*
+         * After continuously processing CHANGES_THRESHOLD changes,
we try to send
+         * a keepalive message if required.
+         *
+         * We don't want to try sending a keepalive message after
processing each
+         * change as that can have overhead. Testing reveals that there is no
+         * noticeable overhead in doing it after continuously
processing 100 or so
+         * changes.
+         */
+#define CHANGES_THRESHOLD 100
+        if (++changes_count >= CHANGES_THRESHOLD)
+        {
+                OutputPluginUpdateProgress(ctx, skipped_xact, end_xact);
+                changes_count = 0;
+        }

Can we merge two if branches since we do the same things? Or did you
separate them for better readability?

Regards,


--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

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