Re: [HACKERS] pgbench tap tests & minor fixes - Mailing list pgsql-hackers

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.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers