On 12/07/18 21:27, Fabien COELHO wrote:
For the testing, we just need to make sure that at least one progress report
is always printed, if -P is used. Right?
Yep. That is the first condition above the last_report is set to
thread_start meaning that there has been no report.
So where does the 0.5 second rule come in? Can't we just do "if (no
progress reports were printed) { print progress report; }" at the end?
The second 0.5s condition is to print a closing report if some time
significant time passed since the last one, but we do not want to print
a report at the same second.
pgbench -T 5 -P 2
Would then print report at 2, 4 and 5. 0.5 ensures that we are not going
to do "2 4.0[00] 4.0[01]" on -t whatever -P 2, which would look silly.
If you do not like it then the second condition can be removed, fine with
me.
As the code stands, you would get reports at "2 4.0[00]", right? Let's
keep it that way. I think the only change we need to make in the logic
is to check at the end, if *any* progress reports at all have been
printed, and print one if not. And do that only when the -P option is
smaller than the -T option, I suppose.
It also adds a small feature which is that there is always a final
progress when the run is completed, which can be useful when computing
progress statistics, otherwise some transactions could not be reported in
any progress at all.
Any transactions in the last 0.5 seconds might still not get reported in any
progress reports.
Yep. I wanted a reasonable threshold, given that both -T and -P are in
seconds anyway, so it probably could only happen with -t.
Oh. I'm a bit surprised we don't support decimals, i.e. -P 0.5.
Actually, it seems to be acceptd, but it's truncated down to the nearest
integer. That's not very nice :-(. But it's a separate issue.
Indeed… but then throttling would not be tested:-) The point of the test
is to exercise all time-related options, including throttling with a
reasonable small value.
Ok. I don't think that's really worthwhile. If we add some code that only
runs in testing, then we're not really testing the real thing. I wouldn't
trust the test to tell much. Let's just leave out that magic environment
variable thing, and try to get the rest of the patch finished.
If you remove the environment, then some checks need to be removed,
because the 2 second run may be randomly shorten when there is nothing to
do. If not, the test will fail underterminiscally, which is not
acceptable. Hence the hack. I agree that it is not beautiful.
The more reasonable alternative could be to always last 2 seconds under
-T 2, even if the execution can be shorten because there is nothing to do
at all, i.e. remove the environment-based condition but keep the sleep.
That sounds reasonable. It's a bit silly to wait when there's nothing to
do, but it's also weird if the test exits before the specified time is
up. Seems less surprising to always sleep.
- Heikki
