Re: pgbench progress report improvements - split 3 v2 - A

On Sun, Oct 06, 2013 at 06:44:11PM +0200, Fabien COELHO wrote:
> >>>Patch (2): Make --initialize mode respect --progress.
> >>>Rejected
> >>
> >>I missed this one...
> >
> >See the second half of this message, including quoted material:
> >http://www.postgresql.org/message-id/CA+TgmoZNXkm-EtszHX=KWq34H5Ni4CS8DG31no86cmDryAqZ_w@mail.gmail.com
>
> I did not read Peter Haas comments as whether --progress should be
> used for both modes, although it is in the section about it.
>
> All his argumentation is about *not changing any defaults*. I
> understood that his "-1" applied about the dropping the row-counting
> based reporting: Robert complains about any "break[ing] backward
> compatibility again one release later" in the paragraph, not really
> about --progress becoming significant under -i, so this would be a
> veto agains what you called "Patch (1)".
>
> So what I've done in version 5 of the patch on this subject is that
> --progress specifies the interval of the progress report in any
> mode, and quiet just set it to five seconds for initialization as is
> the current behavior. Version 5 does not change any current default
> behavior, as I understood that this was the requirement expressed by
> Robert Haas.

I don't know whether that accurately describes Robert's position, but I will
elaborate on my own position:

pgbench already offers two schedules of "pgbench --initialize" messaging,
message-per-100k-rows and message-per-5s. A user too picky to find
satisfaction in either option can filter the messages through grep, sed et al.
We patched pgbench on two occasions during the 9.3 cycle to arrive at that
status quo. Had I participated, I may well have voted for something like your
proposal over "pgbench --quiet". Now that we've released --quiet, a proposal
for an additional message schedule option needs to depict a clear and
convincing step forward. This proposal does not rise to that level.

> >>>Patch (5): Take thread start time at the beginning of the thread.
> >>>Returned with Feedback
> >>
> >>Hmmm. I fought back on the feedback:-) I thought my arguments where
> >>good enough to consider an accept.
> >
> >Here is the feedback in question:
> >http://www.postgresql.org/message-id/20130930223621.GA125986@tornado.leadboat.com
> >
> >With or without the patch, reported performance figures are uninformative when
> >thread start time is substantial relative to benchmark duration. A mere time
> >accounting change will not help much; improving this requires tighter
> >synchronization around the start of actual query activity across threads. I
> >didn't read anything in your response as disagreement with that conclusion.
>
> In my answer to the message you mention.
>
> http://www.postgresql.org/message-id/alpine.DEB.2.02.1310011422130.324@sto
>
> I explained why I had to re-take gettimeofday under --rate because
> the start_time value cannot be relied on. The code I suggested
> simplifies the logic by taking the time only once, instead of
> ignoring the first one because it does not make sense. It seems to
> me that the logical answer to this argument could have been "ok".
> But it seems that you do not percieve my logic.

Yeah, perhaps I don't understand. Throttling behaves fine, because pgbench
already does take a fresh timestamp in threadRun(). Your patch makes
thread->start_time adequate in place of that fresh timestamp. If that were
the patch's only effect, it would be okay as a minor code cleanup. That is
not the patch's only effect. Its other effect, explored thoroughly in my post
referenced above, changes one bad user-visible behavior to a different bad
user-visible behavior. When a code cleanup has such a side effect, we don't
commit it.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com