Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" testpending solution of its timing is (fwd) - Mailing list pgsql-hackers
From | Heikki Linnakangas |
---|---|
Subject | Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" testpending solution of its timing is (fwd) |
Date | |
Msg-id | 9af2f1bf-d3e5-f0eb-505f-7967ee9a88ec@iki.fi Whole thread Raw |
In response to | Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" testpending solution of its timing is (fwd) (Fabien COELHO <coelho@cri.ensmp.fr>) |
Responses |
Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" testpending solution of its timing is (fwd)
(Fabien COELHO <coelho@cri.ensmp.fr>)
|
List | pgsql-hackers |
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
pgsql-hackers by date: