MIME-Version: 1.0
References: <CAA4eK1+fQjndoBOFUn9Wy0hhm3MLyUWEpcT9O7iuCELktfdBiQ@mail.gmail.com>
 <CAA4eK1LGnaPuWs2M4sDfpd6JQZjoh4DGAsgUvNW=Or8i9z6K8w@mail.gmail.com>
 <OS3PR01MB62756599B78FA7D4C908109A9E179@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1JKZrOfCLMVZYfUnm_wfxucDjOzeGwQy3hJ_U9Y3AV05Q@mail.gmail.com>
 <OS3PR01MB627542044A4C16511E63D5DA9E1A9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAD21AoCEhZ6D1VtqvxK1vO0=NnJd6phWbijt-w=M5hwka=-c1A@mail.gmail.com>
 <OS3PR01MB62750C20AD7AF4F13541FC109E1A9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <TYAPR01MB586636517DED745651D6E75AF51D9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <OS3PR01MB6275C64F264662E84D2FB7AE9E1D9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS3PR01MB6275F0E4E27F5522847589ED9E1E9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS3PR01MB6275E0C2B4D9E488AD7CBA209E1F9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1JUgO9F5fZb5P8quMYfDbD7SBNApsTsGLf6vYNNetpWTA@mail.gmail.com>
 <CAD21AoCQi33mjd=bM0Jhkk3QaV-fHVB_soA7iQmTor2A-1Cx3Q@mail.gmail.com>
 <0271e197-2cfe-4627-9c11-47535cc7fdf3@www.fastmail.com> <CAA4eK1+wSvfDVUSe-4o6APFdO8=9G-HnZJ+01_LQOxqLmhqe5Q@mail.gmail.com>
 <8cc4f31f-240a-4590-94a8-a50bdf4f4d1c@www.fastmail.com> <CAA4eK1+HNmgtWRm2U7EG2T0yWbAvhqSDDFBmKX5+vSgBsRmY=w@mail.gmail.com>
 <OS3PR01MB6275FD2CE0850FC66512D8F99EE79@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1+A2aesu8YS9Y8bFBCbOUw_UkU1Nj_-V6rfXBiJgCxqKg@mail.gmail.com>
 <CAA4eK1L-MU+YfRUD0J1wBu6CJ2FEaSVh=Kr3PjjyshprUpvkeg@mail.gmail.com>
 <OS3PR01MB627516D7A3A2ED91EEAA510C9EE79@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1L8jDBGqdZU0Fk6LDSpdqE+3wg2AwkgdxwiCqmXHc6aoQ@mail.gmail.com>
 <OS3PR01MB62755D216245199554DDC8DB9EEA9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1Kj=-5opwBstZtu2CBDMtoQPmDD4c=X5arqx54wq52FNw@mail.gmail.com>
 <CAD21AoC-VnFSB8WHaXpD-X1h7Kz0DVs8C+tVSdPGw+b=BgJG1w@mail.gmail.com>
 <CAA4eK1Ly6eA4ui60mABgtQki0XRLPcSqjQngMTXHGPjjZOtCHQ@mail.gmail.com>
 <CAD21AoDnyjNnEjgeC6ALKwCNXcqtz7SRPgDA-0HV8gguLdLDHw@mail.gmail.com> <OS3PR01MB627561344A2C7ECF68E41D7E9EF39@OS3PR01MB6275.jpnprd01.prod.outlook.com>
In-Reply-To: <OS3PR01MB627561344A2C7ECF68E41D7E9EF39@OS3PR01MB6275.jpnprd01.prod.outlook.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 19 Apr 2022 10:32:07 +0900
Message-ID: <CAD21AoCLaC-Dj=dcz5hQqcxpzi7h_eDsV5uc2156LkrKa6mLQw@mail.gmail.com>
Subject: Re: Logical replication timeout problem
To: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>
Cc: 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"
Archived-At: <https://www.postgresql.org/message-id/CAD21AoCLaC-Dj%3Ddcz5hQqcxpzi7h_eDsV5uc2156LkrKa6mLQw%40mail.gmail.com>
Precedence: bulk

On Mon, Apr 18, 2022 at 3:16 PM wangw.fnst@fujitsu.com
<wangw.fnst@fujitsu.com> wrote:
>
> On Mon, Apr 18, 2022 at 00:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Mon, Apr 18, 2022 at 1:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Apr 14, 2022 at 5:50 PM Masahiko Sawada <sawada.mshk@gmail.com>
> > wrote:
> > > >
> > > > 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?
> > > >
> > >
> > > Do you intend to say update_replication_progress()? The word
> > > 'replation' doesn't make sense to me. I am fine with this suggestion.
> >
> > Yeah, that was a typo. I meant update_replication_progress().
> Thanks for your comments.
>
> > > > 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?
> Improve as suggested. Change the name from update_progress to
> update_replication_progress.
>
> > > > ---
> > > > +        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?
> Improve as suggested. Merge two if-branches.
>
> Attach the new patch.
> 1. Rename the new function(update_progress) to update_replication_progress. [suggestion by Sawada-San]
> 2. Merge two if-branches in new function update_replication_progress. [suggestion by Sawada-San.]
> 3. Improve comments to make them clear. [suggestions by Euler-San.]

Thank you for updating the patch.

+ * For a large transaction, if we don't send any change to the downstream for a
+ * long time(exceeds the wal_receiver_timeout of standby) then it can timeout.
+ * This can happen when all or most of the changes are either not published or
+ * got filtered out.

+ */
+ if(end_xact || ++changes_count >= CHANGES_THRESHOLD)
+ {

We need a whitespace before '(' at above two places. The rest looks good to me.

Regards,

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