Re: pgbench stats per script & other stuff

On Sat, Oct 3, 2015 at 3:11 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>
>> Here is a v10, which is a rebase because of the "--progress-timestamp"
>> option addition.
>
>
> Here is a v11, which is a rebase after some recent changes committed to
> pgbench.

+ The provided <repleacable>scriptname</> needs only to be a prefix
s/repleacable/replaceable, in short I think that documentation
compilation would fail.

- Do not update <structname>pgbench_tellers</> and
- <structname>pgbench_branches</>.
- This will avoid update contention on these tables, but
- it makes the test case even less like TPC-B.
+ Shorthand for <option>-b simple-update(at)1</>.
I don't think it is a good idea to remove entirely the description of
what the default scenarios can do. The description would be better at
the bottom in some <para> with a list of each default test and what to
expect from them.

+/* data structure to hold various statistics.
+ * it is used for interval statistics as well as file statistics.
*/
Nitpick: this is not a comment formatted the Postgres-way.

This is surprisingly broken:
$ pgbench -i
some of the specified options cannot be used in initialization (-i) mode

Any file name or path including "@" will fail strangely:
$ pgbench -f "test(at)1(dot)sql"
could not open file "test": No such file or directory
empty commands for test
Perhaps instead of failing we should warn the user and enforce the
weight to be set at 1?

$ pgbench -b foo
no builtin found for "foo"
This is not really helpful for the user, I think that the list of
potential options should be listed as an error hint.

- " -N, --skip-some-updates skip updates of
pgbench_tellers and pgbench_branches\n"
+ " -N, --skip-some-updates same as \"-b simple-update(at)1\"\n"
" -P, --progress=NUM show thread progress
report every NUM seconds\n"
" -r, --report-latencies report average latency
per command\n"
" -R, --rate=NUM target rate in
transactions per second\n"
" -s, --scale=NUM report this scale
factor in output\n"
- " -S, --select-only perform SELECT-only
transactions\n"
+ " -S, --select-only same as \"-b select-only(at)1\"\n"
It is good to mention that there is an equivalent, but I think that
the description should be kept.

+ /* although a mutex would make sense, the
likelyhood of an issue
+ * is small and these are only stats which may
be slightly false
+ */
+ doSimpleStats(& commands[st->state]->stats,
+ INSTR_TIME_GET_DOUBLE(now) -
+
INSTR_TIME_GET_DOUBLE(st->stmt_begin));
Why would the likelyhood of an issue be small here?

+ /* print NaN if no transactions where executed */
+ double latency = ss->sum / ss->count;
This does not look like a good idea, ss->count can be 0.

It seems also that it would be a good idea to split the patch into two parts:
1) Refactor the code so as the existing test scripts are put under the
same umbrella with addScript, adding at the same time the new option
-b.
2) Add the weight facility and its related statistics.

The patch having some issues, I am marking it as returned with
feedback. It would be nice to see a new version for next CF.
Regards,
--
Michael