Re: pg_receivexlog --status-interval add fsync feedback - Mailing list pgsql-hackers

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Nov 18, 2014 at 2:36 AM, Fujii
Masao<span dir="ltr"><<a href="mailto:masao.fujii@gmail.com" target="_blank">masao.fujii@gmail.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="HOEnZb"><divclass="h5">On Thu, Nov 13, 2014 at 12:38 PM, Fujii Masao <<a
href="mailto:masao.fujii@gmail.com">masao.fujii@gmail.com</a>>wrote:<br /> > Thanks for reviewing the patch!<br
/>><br /> > On Thu, Nov 13, 2014 at 4:05 AM, Alvaro Herrera<br /> > <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br /> >> Fujii Masao wrote:<br />
>><br/> >>> --- 127,152 ----<br /> >>>            When this option is used,
<application>pg_receivexlog</>will report<br /> >>>            a flush position to the server,
indicatingwhen each segment has been<br /> >>>            synchronized to disk so that the server can remove
thatsegment if it<br /> >>> !          is not otherwise needed. <literal>--synchronous</literal>
optionmust<br /> >>> !         be specified when making <application>pg_receivexlog</> run as<br
/>>>> !         synchronous standby by using replication slot. Otherwise WAL data<br /> >>> !       
 cannotbe flushed frequently enough for this to work correctly.<br /> >>>           </para><br />
>>>        </listitem><br /> >>>        </varlistentry><br /> >><br /> >>
Whitespacedamage here.<br /> ><br /> > Fixed.<br /> ><br /> >>> +     printf(_("      --synchronous 
   flush transaction log in real time\n"));<br /> >><br /> >> "in real time" sounds odd.  How about "flush
transactionlog<br /> >> immediately after writing", or maybe "have transaction log writes be<br /> >>
synchronous".<br/> ><br /> > The former sounds better to me. So I chose it.<br /> ><br /> >>> ---
781,791----<br /> >>>               now = feGetCurrentTimestamp();<br /> >>><br /> >>>     
        /*<br /> >>> !              * Issue sync command as soon as there are WAL data which<br />
>>>!              * has not been flushed yet if synchronous option is true.<br /> >>>               
*/<br/> >>>               if (lastFlushPosition < blockpos &&<br /> >>> !                 
  walfile != -1 && synchronous)<br /> >><br /> >> I'd put the "synchronous" condition first in the
if(),and start the<br /> >> comment with it rather than putting it at the end.  Both seem weird.<br /> ><br />
>Fixed, i.e., moved the "synchronous" condition first in the if()'s test<br /> > and also moved the comment "If
synchronousoption is true" also first<br /> > in the comment.<br /> ><br /> >>>                         
                    progname, current_walfile_name, strerror(errno));<br /> >>>                             
 gotoerror;<br /> >>>                       }<br /> >>>                       lastFlushPosition =
blockpos;<br/> >>> !<br /> >>> !                     /*<br /> >>> !                      *
Sendfeedback so that the server sees the latest WAL locations<br /> >>> !                      * immediately
ifsynchronous option is true.<br /> >>> !                      */<br /> >>> !                     if
(!sendFeedback(conn,blockpos, now, false))<br /> >>> !                             goto error;<br />
>>>!                     last_status = now;<br /> >><br /> >> I'm not clear about this comment ..
whydoes it say "if synchronous<br /> >> option is true" when it's not checking the condition?<br /> ><br />
>I added that comment because the code exists with the if() block<br /> > checking "synchronous" condition. But
itseems confusing. Just removed<br /> > that part from the comment.<br /> ><br /> > Attached is the updated
versionof the patch.<br /><br /></div></div>I've just pushed this.<br /></blockquote></div>Marked this patch as
committedon the commit fest app.<br />-- <br /><div class="gmail_signature">Michael<br /></div></div></div>