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

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.