Re: pgbench stats per script & other stuff

> - 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.

I'm trying to avoid to have the same explanation twice, otherwise someone
is bound to complain.

> +/* 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.

Indeed.

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

Hmmm.

> 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?

Yep, I can have a look at that.

> $ 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.

Yep.

> - " -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.

The reason replace it is to keep the help message short column-wise.

> + /* 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) -

> Why would the likelyhood of an issue be small here?

The time to update one stat (<< 100 cycles ?) to the time to do a
transaction with the database (typically Y ms), so the likelyhood of two
thread to update the very same stat at the same time is probably under
1/10,000,000. Even if it occurs, then one stat is slightly false, no big
deal. So I think the potential slowdown induced by a mutex is not worth
it, so I a comment instead.

> + /* 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.

"sum" is a double so count is converted to 0.0, 0.0/0.0 == NaN, hence the
comment.

> 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.

Sigh. The patch & documentation are probably not independent, so that
would make two dependent patches, probably.

--
Fabien.