User-Agent: Cyrus-JMAP/3.7.0-alpha0-382-g88b93171a9-fm-20220330.001-g88b93171
Mime-Version: 1.0
Message-Id: <8cc4f31f-240a-4590-94a8-a50bdf4f4d1c@www.fastmail.com>
In-Reply-To: 
 <CAA4eK1+wSvfDVUSe-4o6APFdO8=9G-HnZJ+01_LQOxqLmhqe5Q@mail.gmail.com>
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>
Date: Thu, 31 Mar 2022 23:57:39 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Amit Kapila" <amit.kapila16@gmail.com>
Cc: "Masahiko Sawada" <sawada.mshk@gmail.com>,
 "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.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>
Subject: Re: Logical replication timeout problem
Content-Type: multipart/alternative;
 boundary=3351d8345bbe44c295507cd1a6481450
Archived-At: <https://www.postgresql.org/message-id/8cc4f31f-240a-4590-94a8-a50bdf4f4d1c%40www.fastmail.com>
Precedence: bulk

--3351d8345bbe44c295507cd1a6481450
Content-Type: text/plain

On Thu, Mar 31, 2022, at 11:27 PM, Amit Kapila wrote:
> This is exactly our initial analysis and we have tried a patch on
> these lines and it has a noticeable overhead. See [1]. Calling this
> for each change or each skipped change can bring noticeable overhead
> that is why we decided to call it after a certain threshold (100) of
> skipped changes. Now, surely as mentioned in my previous reply we can
> make it generic such that instead of calling this (update_progress
> function as in the patch) for skipped cases, we call it always. Will
> that make it better?
That's what I have in mind but using a different approach.

> > The functions CreateInitDecodingContext and CreateDecodingContext receives the
> > update_progress function as a parameter. These functions are called in 2
> > places: (a) streaming replication protocol (CREATE_REPLICATION_SLOT) and (b)
> > SQL logical decoding functions (pg_logical_*_changes). Case (a) uses
> > WalSndUpdateProgress as a progress function. Case (b) does not have one because
> > it is not required -- local decoding/communication. There is no custom update
> > progress routine for each output plugin which leads me to the question:
> > couldn't we encapsulate the update progress call into the callback functions?
> >
> 
> Sorry, I don't get your point. What exactly do you mean by this?
> AFAIS, currently we call this output plugin API in pgoutput functions
> only, do you intend to get it invoked from a different place?
It seems I didn't make myself clear. The callbacks I'm referring to the
*_cb_wrapper functions. After every ctx->callbacks.foo_cb() call into a
*_cb_wrapper() function, we have something like:

if (ctx->progress & PGOUTPUT_PROGRESS_FOO)
    NewUpdateProgress(ctx, false);

The NewUpdateProgress function would contain a logic similar to the
update_progress() from the proposed patch. (A different function name here just
to avoid confusion.)

The output plugin is responsible to set ctx->progress with the callback
variables (for example, PGOUTPUT_PROGRESS_CHANGE for change_cb()) that we would
like to run NewUpdateProgress.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--3351d8345bbe44c295507cd1a6481450
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Thu, Mar 31,=
 2022, at 11:27 PM, Amit Kapila wrote:<br></div><blockquote type=3D"cite=
" id=3D"qt" style=3D""><div>This is exactly our initial analysis and we =
have tried a patch on<br></div><div>these lines and it has a noticeable =
overhead. See [1]. Calling this<br></div><div>for each change or each sk=
ipped change can bring noticeable overhead<br></div><div>that is why we =
decided to call it after a certain threshold (100) of<br></div><div>skip=
ped changes. Now, surely as mentioned in my previous reply we can<br></d=
iv><div>make it generic such that instead of calling this (update_progre=
ss<br></div><div>function as in the patch) for skipped cases, we call it=
 always. Will<br></div><div>that make it better?<br></div></blockquote><=
div>That's what I have in mind but using a different approach.<br></div>=
<div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>&gt; =
The functions CreateInitDecodingContext and CreateDecodingContext receiv=
es the<br></div><div>&gt; update_progress function as a parameter. These=
 functions are called in 2<br></div><div>&gt; places: (a) streaming repl=
ication protocol (CREATE_REPLICATION_SLOT) and (b)<br></div><div>&gt; SQ=
L logical decoding functions (pg_logical_*_changes). Case (a) uses<br></=
div><div>&gt; WalSndUpdateProgress as a progress function. Case (b) does=
 not have one because<br></div><div>&gt; it is not required -- local dec=
oding/communication. There is no custom update<br></div><div>&gt; progre=
ss routine for each output plugin which leads me to the question:<br></d=
iv><div>&gt; couldn't we encapsulate the update progress call into the c=
allback functions?<br></div><div>&gt;<br></div><div><br></div><div>Sorry=
, I don't get your point. What exactly do you mean by this?<br></div><di=
v>AFAIS, currently we call this output plugin API in pgoutput functions<=
br></div><div>only, do you intend to get it invoked from a different pla=
ce?<br></div></blockquote><div>It seems I didn't make myself clear. The =
callbacks I'm referring to the<br></div><div>*_cb_wrapper functions. Aft=
er every ctx-&gt;callbacks.foo_cb() call into a<br></div><div>*_cb_wrapp=
er() function, we have something like:<br></div><div><br></div><div>if (=
ctx-&gt;progress &amp; PGOUTPUT_PROGRESS_FOO)<br></div><div>&nbsp;&nbsp;=
&nbsp; NewUpdateProgress(ctx, false);<br></div><div><br></div><div>The N=
ewUpdateProgress function would contain a logic similar to the<br></div>=
<div>update_progress() from the proposed patch. (A different function na=
me here just<br></div><div>to avoid confusion.)<br></div><div><br></div>=
<div>The output plugin is responsible to set ctx-&gt;progress with the c=
allback<br></div><div>variables (for example, PGOUTPUT_PROGRESS_CHANGE f=
or change_cb()) that we would<br></div><div>like to run NewUpdateProgres=
s.<br></div><div><br></div><div><br></div><div id=3D"sig112409080"><div =
class=3D"signature">--<br></div><div class=3D"signature">Euler Taveira<b=
r></div><div class=3D"signature">EDB&nbsp;&nbsp; <a href=3D"https://www.=
enterprisedb.com/">https://www.enterprisedb.com/</a><br></div></div><div=
><br></div></body></html>
--3351d8345bbe44c295507cd1a6481450--