Re: pgbench stats per script & other stuff - Mailing list pgsql-hackers

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

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

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