Re: pgbench stats per script & other stuff

On Sat, Oct 3, 2015 at 3:11 PM, Fabien COELHO <coelho@cri.ensmp.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@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@1.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@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@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