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


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


-                  "  -S, --select-only        perform SELECT-only
+                  "  -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
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.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to