From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pgbench stats per script & other stuff |
Date: | 2016-02-04 15:53:55 |
Message-ID: | alpine.DEB.2.10.1602041520590.20461@sto |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello Alvaro,
> Something is wrong with patch d. I noticed two things,
>
> 1. the total_weight stuff can overflow,
It can generate an error on overflow by checking the total_weight while
it is being computed. I've switched total_weight to int64 so it is now
really impossible to overflow with the 32 bit int_max limit on weight.
> 2. the chooseScript stuff is broken, or something.
Sorry, probably a <=/< error. I think I've fixed it and I've simplified
the code a little bit.
> Another thing is that the "transaction type" output really deserves some
> more work. I think "multiple scripts" really doesn't cut it; we should
> have some YAML-like as in the latency reports, which lists all scripts
> in use and their weights.
For me the current output is clear for the reader, which does not
mean that it cannot be improve, but how is rather a matter of taste.
I've tried to improve it further, see attached patch.
If you want something else, it would help to provide a sample of what you
expect.
> Also, while I have your attention regarding accumulated "technical
> debt", please have a look at the "desc" argument used in addScript etc.
> It's pretty ridiculous currently. Maybe findBuiltin / process_builtin /
> process_file should return a struct containing Command ** and the
> "desc" string, rather than passing desc as a separate argument.
Ok, it can return a pointer to the builtin script.
> Attached is my version of the patch. While you're messing with it, it'd
> be nice if you added comments on top of your recently added functions
> such as findBuiltin, process_builtin, chooseScript.
Why not.
Find attached a 18-d which addresses these concerns, and a actualized 18-e
for the prefix.
--
Fabien.
Attachment | Content-Type | Size |
---|---|---|
pgbench-script-stats-18-d.patch | text/x-diff | 12.8 KB |
pgbench-script-stats-18-e.patch | text/x-diff | 2.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-02-04 15:54:58 | Re: checkpointer continuous flushing - V16 |
Previous Message | Michael Paquier | 2016-02-04 15:38:28 | Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby |