Re: pgbench tap tests & minor fixes

Hello Nikolay,

> Year, this is much more clear for me. Now I understand this statistics code.

Great.

> I still have three more questions. A new one:
>
> ========
> my_command->line = expr_scanner_get_substring(sstate,
> start_offset,
> - end_offset);
> + end_offset + 1);
> ========
>
> I do not quite understand what are you fixing here,

I fix a truncation which appears in an error message later on.

> I did not find any mention of it in the patch introduction letter.

Indeed. Just a minor bug fix to avoid an error message to be truncated. If
you remove it, then you can get:

missing argument in command "sleep"
\slee

Note the missing "p"...

> And more, expr_scanner_get_substring is called twice in very similar
> code, and it is fixed only once. Can you tell more about this fix.

I fixed the one which was generating truncated messages. I did not notice
other truncated messages while testing, so I assume that other calls are
correct, but I did not investigate further, so I may be wrong. Maybe in
other instances the truncation removes a "\n" which is intended?

> Old one:
>
> (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */
> if (progress_timestamp && progress <= 0)

>> I am still sure that the right way is to do '== 0' and Assert for case
>> when it is negative.

- nxacts is a counter, it could wrap around at least theoretically.

- progress is checked for >=0, so ==0 is fine.

Note that the same trick is used in numerous places in pgbench code, and I
did not wrote most of them:

st->nvariables <= 0
duration <= 0
total->cnt <= 0
(nxacts <= 0 && duration <= 0)

> If you are sure it is good to do '<= 0', let's allow commiter to do final
> decision.

I'm sure that it is a minor thing, and that the trick is already used in
the code. I changed progress because there is a clearly checked, but I
kept nxacts because of the theoritical wrap around.

> And another unclosed issue:
>
> I still do not like command_checks_all function name (I would prefer
> command_like_more) but I can leave it for somebody more experienced (i.e.
> commiter) to make final decision, if you do not agree with me here...

I've propose the name because it checks for everything (multiple stdout,
multiple stderr, status), hence "all". The "like" just refers to stdout
regex, so is quite limited, and "checks all" seems to be a good summary of
what is done, while "like more" is pretty unclear to me, because it is
relative to "like", so I have to check what "like" does and then assume
that it does more...

> /* Why I am so bothered with function name. We are adding this function to
> library that are used by all TAP-test-writers. So here things should be 100%
> clear for all.

Yep. "checks_all" is clearer to me that "like_more" which is relative to
another function.

> If this function was only in pgbench test code, I would not
> care about the name at all. But here I do. I consider it is important to give
> best names to the functions in shared libraries. */
>
> Hope these are last one. Let's close the first issue, fix or leave unclosed
> others, and finish with this patch :-)

Here is a v6.

- it uses "progress == 0"

- it does not change "nxacts <= 0" because of possible wrapping

- it fixes two typos in a perl comment about the checks_all function

- I kept "checks all" because this talks more to me than "like more"
if a native English speaker or someone else has an opinion, it is
welcome.

Also, if someone could run a test on windows, it would be great.

--
Fabien.