Re: pg_receivexlog --status-interval add fsync feedback

On Wed, Nov 19, 2014 at 10:20 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Tue, Nov 18, 2014 at 2:36 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>> On Thu, Nov 13, 2014 at 12:38 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> wrote:
>> > Thanks for reviewing the patch!
>> >
>> > On Thu, Nov 13, 2014 at 4:05 AM, Alvaro Herrera
>> > <alvherre(at)2ndquadrant(dot)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