Re: pgbench tap tests & minor fixes

Hello Nikolay,

>> Here is a v3, with less files. I cannot say I find it better, but it
>> still works.
>
> Actually I like it. It is what I expect it to be ;-) [...]

Well, if someone is happy about the code, that's good:-)

>> The "command_likes" function has been renamed "command_checks".
>
> This is one thing have my doubts about. [...]
> perl experience tells me that this new function should be called
> command_likes_more. It is a kind of tradition in test naming things
> "simple, more, most, etc".

Hmmm. I've done such things in the past, but it felt more like a lack of
imagination than good naming practices:-)

What about "command_checks_all"?

> 1. I quite agree with Robert, that this options patch is a separate issue and
> it would be better do deal with them separately. But I wild not insist. It
> will just made our work on this commit longer, instead of two shorter works.

Yep. I'm fine with the patch being committed in two parts, one for the C
file and the perl test separately. However the perl test would not pass
without the C changes, so it would be commit C changes and then the tests.

...
> while (thread->throttle_trigger < now_us - latency_limit &&
> /* with -t, do not overshoot */
> (nxacts <= 0 || st->cnt < nxacts))
...
> if (nxacts > 0 && st->cnt >= nxacts)
> {
> st->state = CSTATE_FINISHED;
> break;
> }
>
> First of all I do not completely understand this while and your changes
> in it.

When under throttling (-R) with latency limit (-L), if the stochastic
process schedules transactions which are already too late, they are
skipped by this while loop. However, if a number of transaction is
specified (-t), this resulted in more transaction being considered in the
report than the number that was asked for, and the displayed statistics
were wrong. The added condition allows to not overshoot the target number
of transactions in that case.

> This while() is inside... Originally It detects that the transaction is late,
> "counts" how many throttle_delay will fit in this total delay, and for each
> throttle_delay do some logging in processXactStats and counts thread->
> latency_late total number there too.

Yep.

> Please note that in original code thread->stats.cnt++; is one only when not
> (progress || throttle_delay || latency_limit)

The same incrementation is done in accumStats function, among other
things, in the other branch.

> And you've added st->cnt ++; with no conditions. So if transaction is delayed
> for N*throttle_delay miliseconds, your st->cnt will be incremented N times.

Yep. This is the number of transactions "considered" by the client, both
those executed and those skipped.

> Are you sure it is what you want to do?

Yes.

> If so, I need more explanations because I do not understand why do you
> want to count it that way, and what exactly do you count here.

There are two counts: one in the threads, and one in the clients. They
were not cleanly accounted for, so I've updated the code so that what is
counted is clearer, and so that thread & client accounting is done in the
same place/functions, and that formula which use these counts are
consistent.

The stats are updated through the processXactsStats/accumStats which
collect many statistics about execution time and so, but this precise
collection is skipped when the stats are not needed in order to avoid some
cycles, and just simple counting is done.

> Why do you check nxacts > 0? It _must_ be grater then 0.

Only under -t (number of transaction). It is 0 under -T (time to run),
which is the usual approach to run tests, but for creating small
deterministic tests I used -t a lot, hence I noticed that some things were
not right.

I could do "nacts == 0" instead of "nacts <= 0", because probably "nacts
>= 0", but it seemed safer with <= which complements > in the while
condition.

> I think it one want to be sure that some value is valid, he uses Assert.

Nope, the test is really needed.

> More notes about code styling: [...]

I've tried to improve comments placement.

> The second style issue: [...]

I like the comma operator from time to time, but I can use {} as well.

Please find attached a v4. To sum it up:

About pgbench.c changes:
- make -t work properly with -R and -L and display sensible stats
- check that --progress-timestamp => --progress

About TAP test changes:
- add an infrastructure to run a command and check for its
exit status, stdout and stderr.
- add many tests to pgbench, achieving nearly full coverage.
360 tests running time under 7 seconds on my laptop
(versus 3 tests which ran in about 4 seconds previously).

One open question is whether some test may fail on Windows, if someone
would test that it would be great!

--
Fabien.