MIME-Version: 1.0
References: <OS3PR01MB6275982DA7BBA25DE88F6E839EF59@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAD21AoDWs2Ru9Zc_jzjrA4k4JUtUDN2_9GktbuAzwHwdJY2myg@mail.gmail.com> <OS0PR01MB5716F30A9F73742BC8EA95A594FD9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: <OS0PR01MB5716F30A9F73742BC8EA95A594FD9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 2 May 2022 11:03:13 +0900
Message-ID: <CAD21AoD7P9UU4nMgrotD_7LDbzpTpKugpwbjq5VUAbriqzVOSg@mail.gmail.com>
Subject: Re: Logical replication timeout problem
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoD7P9UU4nMgrotD_7LDbzpTpKugpwbjq5VUAbriqzVOSg%40mail.gmail.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 7:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, April 20, 2022 3:21 PM Masahiko Sawada <sawada.mshk@gmail.c=
om> wrote:
> >
> > BTW the changes in
> > REL_14_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch,
> > adding end_xact to LogicalDecodingContext, seems good to me and it
> > might be better than the approach of v17 patch from plugin developers=
=E2=80=99
> > perspective? This is because they won=E2=80=99t need to pass true/false=
 to
> > end_xact of  OutputPluginUpdateProgress(). Furthermore, if we do what
> > we do in update_replication_progress() in
> > OutputPluginUpdateProgress(), what plugins need to do will be just to
> > call OutputPluginUpdate() in every callback and they don't need to
> > have the CHANGES_THRESHOLD logic. What do you think?
>
> Hi Sawada-san, Wang
>
> I was looking at the patch and noticed that we moved some logic from
> update_replication_progress() to OutputPluginUpdateProgress() like
> your suggestion.
>
> I have a question about this change. In the patch we added some
> restriction in this function OutputPluginUpdateProgress() like below:
>
> + /*
> + * If we are at the end of transaction LSN, update progress tracking.
> + * Otherwise, after continuously processing CHANGES_THRESHOLD changes, w=
e
> + * try to send a keepalive message if required.
> + */
> + if (ctx->end_xact || ++changes_count >=3D CHANGES_THRESHOLD)
> + {
> + ctx->update_progress(ctx, ctx->write_location, ctx->write_xid,
> + skipped_xact);
> + changes_count =3D 0;
> + }
>
> After the patch, we won't be able to always invoke the update_progress() =
if the
> caller are at the middle of transaction(e.g. end_xact =3D false). The beb=
avior of the
> public function OutputPluginUpdateProgress() is changed. I am thinking is=
 it ok to
> change this at back-branches ?
>
> Because OutputPluginUpdateProgress() is a public function for the extensi=
on
> developer, just a little concerned about the behavior change here.

Good point.

As you pointed out, it would not be good if we change the behavior of
OutputPluginUpdateProgress() in back branches. Also, after more
thought, it is not a good idea even for HEAD since there might be
background workers that use logical decoding and the timeout checking
might not be relevant at all with them.

BTW, I think you're concerned about the plugins that call
OutputPluginUpdateProgress() at the middle of the transaction (i.e.,
end_xact =3D false). We have the following change that makes the
walsender not update the progress at the middle of the transaction. Do
you think it is okay since it's not common usage to call
OutputPluginUpdateProgress() at the middle of the transaction by the
plugin that is used by the walsender?

 #define WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS 1000
-     if (!TimestampDifferenceExceeds(sendTime, now,
+     if (end_xact && TimestampDifferenceExceeds(sendTime, now,
      WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS))
-         return;
+     {
+         LagTrackerWrite(lsn, now);
+         sendTime =3D now;
+     }

Regards,

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