Re: pg_receivexlog --status-interval add fsync feedback

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.

Regards,

--
Fujii Masao