Re: pg_receivexlog --status-interval add fsync feedback

On Wed, Nov 19, 2014 at 10:20 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>
>
> On Tue, Nov 18, 2014 at 2:36 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Thu, Nov 13, 2014 at 12:38 PM, Fujii Masao <masao.fujii@gmail.com>
>> wrote:
>> > Thanks for reviewing the patch!
>> >
>> > On Thu, Nov 13, 2014 at 4:05 AM, Alvaro Herrera
>> > <alvherre@2ndquadrant.com> wrote:
>> >> Fujii Masao wrote:
>> >>
>> >>> --- 127,152 ----
>> >>>            When this option is used, <application>pg_receivexlog</>
>> >>> will report
>> >>>            a flush position to the server, indicating when each
>> >>> segment has been
>> >>>            synchronized to disk so that the server can remove that
>> >>> segment if it
>> >>> !          is not otherwise needed. <literal>--synchronous</literal>
>> >>> option must
>> >>> !         be specified when making <application>pg_receivexlog</> run
>> >>> as
>> >>> !         synchronous standby by using replication slot. Otherwise WAL
>> >>> data
>> >>> !         cannot be flushed frequently enough for this to work
>> >>> correctly.
>> >>>           </para>
>> >>>         </listitem>
>> >>>        </varlistentry>
>> >>
>> >> Whitespace damage here.
>> >
>> > Fixed.
>> >
>> >>> +     printf(_("      --synchronous      flush transaction log in real
>> >>> time\n"));
>> >>
>> >> "in real time" sounds odd.  How about "flush transaction log
>> >> immediately after writing", or maybe "have transaction log writes be
>> >> synchronous".
>> >
>> > The former sounds better to me. So I chose it.
>> >
>> >>> --- 781,791 ----
>> >>>               now = feGetCurrentTimestamp();
>> >>>
>> >>>               /*
>> >>> !              * Issue sync command as soon as there are WAL data
>> >>> which
>> >>> !              * has not been flushed yet if synchronous option is
>> >>> true.
>> >>>                */
>> >>>               if (lastFlushPosition < blockpos &&
>> >>> !                     walfile != -1 && synchronous)
>> >>
>> >> I'd put the "synchronous" condition first in the if(), and start the
>> >> comment with it rather than putting it at the end.  Both seem weird.
>> >
>> > Fixed, i.e., moved the "synchronous" condition first in the if()'s test
>> > and also moved the comment "If synchronous option is true" also first
>> > in the comment.
>> >
>> >>>                                               progname,
>> >>> current_walfile_name, strerror(errno));
>> >>>                               goto error;
>> >>>                       }
>> >>>                       lastFlushPosition = blockpos;
>> >>> !
>> >>> !                     /*
>> >>> !                      * Send feedback so that the server sees the
>> >>> latest WAL locations
>> >>> !                      * immediately if synchronous option is true.
>> >>> !                      */
>> >>> !                     if (!sendFeedback(conn, blockpos, now, false))
>> >>> !                             goto error;
>> >>> !                     last_status = now;
>> >>
>> >> I'm not clear about this comment .. why does it say "if synchronous
>> >> option is true" when it's not checking the condition?
>> >
>> > I added that comment because the code exists with the if() block
>> > checking "synchronous" condition. But it seems confusing. Just removed
>> > that part from the comment.
>> >
>> > Attached is the updated version of the patch.
>>
>> I've just pushed this.
>
> Marked this patch as committed on the commit fest app.

Thanks!

Regards,

-- 
Fujii Masao