Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

On Mon, Sep 07, 2020 at 10:06:57AM +0200, Peter Eisentraut wrote:
> However, I see that in the case of pg_test_fsync you end up in alarm(0),
> which does something different, so it's okay in that case to disallow it.

Yep.

> I notice that the error checking you introduce is different from the checks
> for pgbench -t and -T (the latter having no errno checks). I'm not sure
> which is correct, but it's perhaps worth making them the same.

pgbench currently uses atoi() to parse the options of -t and -T. Are
you suggesting to switch that to strtoXX() as well or perhaps you are
referring to the parsing of the weight in parseScriptWeight()? FWIW,
the error handling introduced in this patch is similar to what we do
for example in pg_resetwal. This has its own problems as strtoul()
would not report ERANGE except for values higher than ULONG_MAX, but
the returned results are stored in 32 bits. We could switch to just
use uint64 where we could of course, but is that really worth it for
such tools? For example, pg_test_timing could overflow the
total_timing calculated if using a too high value, but nobody would
use such values anyway. So I'd rather just use uint32 and call it a
day, for simplicity's sake mainly..

> (pgbench -t 0, which is also currently not allowed, is a good example of why
> this could be useful, because that would allow checking whether the script
> etc. can be loaded without running an actual test.)

Perhaps. That looks like a separate item to me though.
--
Michael