Re: pg_receivexlog --status-interval add fsync feedback

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.
--
Michael