On Fri, Feb 5, 2016 at 12:53 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: >> 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.
+ /* compute total_weight */ + for (i = 0; i < num_scripts; i++) + { + total_weight += sql_script[i].weight; + + /* detect overflow... */ + if (total_weight < 0) + { + fprintf(stderr, "script weight overflow at script %d\n", i+1); + exit(1); + } + } If let as int64, you may want to remove this overflow check, or keep it as int32. >> 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. + w = getrand(thread, 0, total_weight - 1); + do + { + w -= sql_script[i++].weight; + } while (w >= 0); + + return i - 1; This portion looks fine to me. >> 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. You could do that with an additional option here as well: --output-format=normal|yamljson. The tastes of each user is different. >> 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. const char *name; + int weight; Command **commands; - StatsData stats; + StatsData stats; Noise here? > Find attached a 18-d which addresses these concerns, and a actualized 18-e > for the prefix. (I could live without that personally) -/* return builtin script "name", or fail if not found */ +/* return commands for selected builtin script, if unambiguous */ static script_t * findBuiltin(const char *name) This comment needs a refresh. This does not return a set of commands, but the script itself. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers