Review of "pg_basebackup and pg_receivexlog to use non-blocking socket communication", was: Re: Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown

<div class="moz-cite-prefix">Hi,<br /><br /> 2012-11-15 14:59 keltezéssel, Amit kapila írta:<br /></div><blockquote
cite="mid:6C0B27F7206C9E4CA54AE035729E9C38285499FA@szxeml509-mbx"type="cite"><pre wrap="">On Monday, November 12, 2012
8:23PM Fujii Masao wrote:
 
On Fri, Nov 9, 2012 at 3:03 PM, Amit Kapila <a class="moz-txt-link-rfc2396E"
href="mailto:amit.kapila@huawei.com"><amit.kapila@huawei.com></a>wrote:
 
</pre><blockquote type="cite"><pre wrap="">On Thursday, November 08, 2012 10:42 PM Fujii Masao wrote:
</pre><blockquote type="cite"><pre wrap="">On Thu, Nov 8, 2012 at 5:53 PM, Amit Kapila <a class="moz-txt-link-rfc2396E"
href="mailto:amit.kapila@huawei.com"><amit.kapila@huawei.com></a>
wrote:
</pre><blockquote type="cite"><pre wrap="">On Thursday, November 08, 2012 2:04 PM Heikki Linnakangas wrote:
</pre><blockquote type="cite"><pre wrap="">On 19.10.2012 14:42, Amit kapila wrote:
</pre><blockquote type="cite"><pre wrap="">On Thursday, October 18, 2012 8:49 PM Fujii Masao wrote:
</pre></blockquote></blockquote></blockquote></blockquote></blockquote><pre wrap="">
</pre><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><pre wrap="">Are you planning to
introducethe timeout mechanism in pg_basebackup
 
main process? Or background process? It's useful to implement both.
</pre></blockquote></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">By background process, you mean ReceiveXlogStream?
For both.
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">I think for background process, it can be done in a way similar to what we
have done for walreceiver.
</pre></blockquote></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">Yes.
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><blockquote type="cite"><pre wrap="">But I have some doubts for how to do for main
process:
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">Logic similar to walreceiver can not be used incase network goes down
during
getting other database file from server.
The reason for the same is to receive the data files PQgetCopyData() is
called in synchronous mode, so it keeps waiting for infinite time till it
gets some data.
In order to solve this issue, I can think of following options:
1. Making this call also asynchronous (but now sure about impact of this).
</pre></blockquote></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">+1
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">Walreceiver already calls PQgetCopyData() asynchronously. ISTM you can
solve the issue in the similar way to walreceiver's.
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><blockquote type="cite"><pre wrap="">2. In function pqWait, instead of passing hard-code
value-1 (i.e. infinite
 
wait), we can send some finite time. This time can be received as command
line argument   from respective utility and set the same in PGconn structure.
</pre></blockquote></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">Yes, I think that we should add something like --conninfo option to
pg_basebackup
and pg_receivexlog. We can easily set not only connect_timeout but also sslmode,
application_name, ... by using such option accepting conninfo string.
</pre></blockquote><pre wrap="">
I have prepared an attached patch to make pg_basebackup and pg_receivexlog as non-blocking.
To do so I have to add new command line parameters in pg_basebackup and pg_receivexlog
for now added two more command line arguments        a.  "-r"  for pg_basebackup and pg_receivexlog to take receive
time-outvalue. Default value for this parameter is 60 sec.        b. "-t"   for pg_basebackup and pg_receivexlog to
takeinitial connection timeout value. Default value is infinite wait. 
 
We can change to accept --conninfo as well. 

I feel apart from above, remaining problem is for function call PQgetResult()
1. Wherever query is getting sent from BaseBackup, it calls the function PQgetResult to receive the result of query.
AsPQgetResult() is blocking function (it calls pqWait which can hang), so if network is down before sending the query
itself,   then there will not be any result, so it will keep hanging in PQgetResult . 
 
IMO, it can be solved in below ways:
a. Create one corresponding non-blocking function. But this function is being called from inside some of the     other
libpqfunction (PQexec->PQexecFinish->PQgetResult). So it can be little tricky to solve this way.
 
b. Add the receive_timeout variable in PGconn structure and use it in pqWait for timeout whenever it is set.
c. any other better way?


</pre><blockquote type="cite"><blockquote type="cite"><pre wrap="">BTW, IIRC the walsender has no timeout mechanism
duringsending
 
backup data to pg_basebackup. So it's also useful to implement the
timeout mechanism for the walsender during backup.
</pre></blockquote><pre wrap="">
</pre></blockquote><pre wrap="">
</pre><blockquote type="cite"><pre wrap="">What about using pq_putmessage_noblock()?
</pre></blockquote><pre wrap="">
I think may be some more functions also needs to be made as noblock. I am still evaluating.

I will upload the attached patch in commitfest if you don't have any objections?

More Suggestions/Comments?

With Regards,
Amit Kapila.</pre></blockquote><br /> I am reviewing your patch.<br /><br /><ul><li> Is the patch in <a class="external
text"href="http://en.wikipedia.org/wiki/Diff#Context_format" rel="nofollow"
title="http://en.wikipedia.org/wiki/Diff#Context_format">contextdiff format</a>? </ul><br /> Yes.<br /><br /><ul><li>
Doesit apply cleanly to the current git master?</ul><br /> Not quite cleanly but it doesn't produce rejects or fuzz,
onlyoffset warnings:<br /><br /> [zozo@localhost postgresql]$ cat ../noblock_basebackup_and_receivexlog.patch | patch
-p1<br/> patching file src/bin/pg_basebackup/pg_basebackup.c<br /> Hunk #1 succeeded at 41 (offset -6 lines).<br />
Hunk#2 succeeded at 123 (offset -6 lines).<br /> Hunk #3 succeeded at 239 (offset -6 lines).<br /> Hunk #4 succeeded at
292(offset -6 lines).<br /> Hunk #5 succeeded at 470 (offset -6 lines).<br /> Hunk #6 succeeded at 588 (offset -6
lines).<br/> Hunk #7 succeeded at 601 (offset -6 lines).<br /> Hunk #8 succeeded at 727 (offset -6 lines).<br /> Hunk
#9succeeded at 779 (offset -6 lines).<br /> Hunk #10 succeeded at 797 (offset -6 lines).<br /> Hunk #11 succeeded at
811(offset -6 lines).<br /> Hunk #12 succeeded at 879 (offset -6 lines).<br /> Hunk #13 succeeded at 1080 (offset -6
lines).<br/> Hunk #14 succeeded at 1381 (offset -6 lines).<br /> Hunk #15 succeeded at 1409 (offset -6 lines).<br />
Hunk#16 succeeded at 1521 (offset -6 lines).<br /> patching file src/bin/pg_basebackup/pg_receivexlog.c<br /> Hunk #1
succeededat 35 (offset -6 lines).<br /> Hunk #2 succeeded at 65 (offset -6 lines).<br /> Hunk #3 succeeded at 224
(offset-6 lines).<br /> Hunk #4 succeeded at 281 (offset -6 lines).<br /> Hunk #5 succeeded at 314 (offset -6
lines).<br/> Hunk #6 succeeded at 341 (offset -5 lines).<br /> Hunk #7 succeeded at 379 (offset -5 lines).<br />
patchingfile src/bin/pg_basebackup/receivelog.c<br /> Hunk #1 succeeded at 181 (offset -9 lines).<br /> Hunk #2
succeededat 201 (offset -9 lines).<br /> Hunk #3 succeeded at 223 (offset -9 lines).<br /> Hunk #4 succeeded at 333
(offset-9 lines).<br /> Hunk #5 succeeded at 342 (offset -9 lines).<br /> Hunk #6 succeeded at 397 (offset -9
lines).<br/> Hunk #7 succeeded at 484 (offset -9 lines).<br /> Hunk #8 succeeded at 533 (offset -9 lines).<br /> Hunk
#9succeeded at 550 (offset -9 lines).<br /> patching file src/bin/pg_basebackup/receivelog.h<br /> patching file
src/bin/pg_basebackup/streamutil.c<br/> Hunk #1 succeeded at 66 (offset -6 lines).<br /> Hunk #2 succeeded at 87
(offset-6 lines).<br /> Hunk #3 succeeded at 118 (offset -6 lines).<br /> patching file
src/bin/pg_basebackup/streamutil.h<br/><br /><ul><li> Does it include reasonable tests, necessary doc patches, etc?
</ul><br/> The test cases are not applicable. There is no test framework for<br /> testing network outage in "make
check".<br/><br /> There are no documentation patches for the new --recvtimeout=INTERVAL<br /> and
--conntimeout=INTERVALoptions for either pg_basebackup or<br /> pg_receivexlog.<br /><br /><ul><li> Does the patch
actuallyimplement that? </ul><p><br /> It seems so, the patch adds the connect_timeout parameter to<br /> the
connectionoptions and uses PQgetCopyData(..., 1) to get<br /> the data asynchronously and uses select(2) to watch for
incoming<br/> data.<br /><br /><ul><li> Do we want that? </ul><p><br /> It can speed up detecting network breakdown so
yes.<br/><br /><ul><li> Do we already have it? </ul><p><br /> No.<br /><br /><ul><li> Does it follow SQL spec, or the
community-agreedbehavior? </ul><p><br /> There's no such SQL spec. The behaviour is desired.<br /><br /><ul><li> Does
itinclude pg_dump support (if applicable)?</ul><p><br /> Not applicable.<br /><br /><ul><li> Are there dangers?
</ul><p><br/> The patch author researched more functions that need<br /> to be extended in a nonblocking way.<br /><a
class="moz-txt-link-freetext"
href="http://archives.postgresql.org/pgsql-hackers/2012-11/msg00863.php">http://archives.postgresql.org/pgsql-hackers/2012-11/msg00863.php</a><br
/><br/><ul><li> Have all the bases been covered? </ul><br /> For pg_basebackup/pg_receivexlog (for PQgetCopyData and<br
/>PQconnect), yes.<br /><br /> Per the previous comment, no. But those are for the backend<br /> to notice network
breakdownsand as such, they need a<br /> separate patch. <br /><br /><ul><li> Does the feature work as
advertised?</ul><p><br/> Yes.<br /><p>I tested it between two machines and pulled the ethernet<br /> plug while
pg_basebackupwas running. With "-r 2", pg_basebackup<br /> detected the timeout after 2 seconds. Without the patch, I
lost<br/> patience after two minutes and pressed Ctrl-C in pg_basebackup.<br /><p>I also tested pg_receivexlog and it
alsonoticed the network error<br /> in the specified timeout.<br /><br /><ul><li> Are there corner cases the author has
failedto consider?</ul><p><br /> As far as I can see in the client-side libpq code flow, no.<br /><br /><ul><li> Are
thereany assertion failures or crashes? </ul><br /> Not applicable, the patch is for client applicatiions.<br /><br
/><ul><li>Does the patch slow down simple tests? </ul><p><br /> No.<br /><br /><ul><li> If it claims to improve
performance,does it?</ul><p><br /> Not applicable, not a performance patch. But it really<br /> improves detecting
networkbreakdown.<br /><br /><ul><li> Does it slow down other things? </ul><br /> No.<br /><br /><ul><li> Does it
followthe project <a class="external text" href="http://developer.postgresql.org/pgdocs/postgres/source.html"
rel="nofollow"title="http://developer.postgresql.org/pgdocs/postgres/source.html">coding guidelines</a>? </ul><p><br />
Yes.<br/><br /><ul><li> Are there portability issues? </ul><p><br /> No. It introduces atoi() and select() as new
calls,these are portable.<br /><br /><ul><li> Will it work on Windows/BSD etc? </ul><p><br /> It should.<br /><br
/><ul><li>Are the comments sufficient and accurate?</ul><p><br /> This chunk below removes a comment which seems
obviousenough<br /> so it's not needed:<br /><p>***************<br /> *** 518,524 **** ReceiveXlogStream(PGconn *conn,
XLogRecPtrstartpos, uint32 timeline,<br />                         goto error;<br />                 }<br />   <br />
!              /* Check the message type. */<br />                 if (copybuf[0] == 'k')<br />                 {<br />
                       int             pos;<br /> --- 559,568 ----<br />                         goto error;<br />
               }<br />   <br /> !               /* Set the last reply timestamp */<br /> !              
last_recv_timestamp= localGetCurrentTimestamp();<br /> !               ping_sent = false;<br /> !               <br />
               if (copybuf[0] == 'k')<br />                 {<br />                         int             pos;<br />
***************<br/><p><br /> Other comments are sufficient and accurate.<br /><br /><ul><li> Does it do what it says,
correctly?</ul><p><br /> This question is redundant with the above "Does the feature work as advertised?"<br /> So
yes.<br/><br /><ul><li> Does it produce compiler warnings?</ul><p><br /> No.<br /><br /><ul><li> Can you make it crash?
</ul><br/> No.<br /><br /><ul><li> Is everything done in a way that fits together coherently with other
features/modules?</ul><p><br /> Yes.<br /><br /><ul><li> Are there interdependencies that can cause problems? </ul><br
/>No.<br /><br /><br /> Best regards,<br /> Zoltán Böszörményi<br /><br /><br /><pre class="moz-signature" cols="90">--

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a>
 
</pre>