From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: pgbench stats per script & other stuff
Date: Mon, 14 Dec 2015 21:53:44 +0100 (CET)
Message-ID: <alpine.DEB.2.10.1512142131430.22108@sto> (raw)
In-Reply-To: <CAB7nPqQV0wHJ4CKw_0YFQbfVahQrw80QfaHRGQw7cgbux7mKCg@mail.gmail.com>
References: <alpine.DEB.2.10.1507220706260.5636@sto>
<WM!36f7f6aea11042e334dd8ddca8dd52df3de0913ec1f5bc42bfcc27f3eb60d576c8dac02c33f02b88abead156dfec3da0!@asav-2.01.com>
<55AFD8C6.8010507@agliodbs.com>
<CA+TgmobZSBmWzugAQp94=q98c2fmSBc+gRPwfNM+94dhjpL3tg@mail.gmail.com>
<alpine.DEB.2.10.1507231759400.10585@sto>
<CA+TgmoZEvNJ8wkVdHCsWnsz8Rz-MHebGp4DY+8WjiU-xYPT6Nw@mail.gmail.com>
<alpine.DEB.2.10.1507250910540.20046@sto>
<alpine.DEB.2.10.1507251618230.25082@sto>
<alpine.DEB.2.10.1507281626330.3538@sto>
<alpine.DEB.2.10.1507301801500.22793@sto>
<20150902172617.GJ5286@alap3.anarazel.de>
<alpine.DEB.2.10.1509030917480.16877@sto>
<alpine.DEB.2.10.1509260923150.8351@sto>
<alpine.DEB.2.10.1510030810440.24488@sto>
<CAB7nPqQV0wHJ4CKw_0YFQbfVahQrw80QfaHRGQw7cgbux7mKCg@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: coelho@cri.ensmp.fr, michael.paquier@gmail.com
Subject: Re: pgbench stats per script & other stuff
In-Reply-To: <alpine.DEB.2.10.1512142131430.22108@sto>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox