Re: pg_receivexlog --status-interval add fsync feedback

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.

Regards,

--
Fujii Masao