*-21.patch does what you suggested above, some hidden awkwardness but much less that the previous one.Yeah, I think this is much nicer, don't you agree?
Yep, I said "less awkwarness than previous", a pretty contrived way to say better:-)
However, this is still a bit broken -- you cannot return a stack variable from process_file, because the stack goes away once the function returns. You need to malloc it.
That is why the "fs" variable in process_file is declared "static", and why I wrote "some hidden awkwarness".
I did want to avoid a malloc because then who would free the struct? addScript cannot to it systematically because builtins are static. Or it would have to create an on purpose struct, but I then that would be more awkwarness, and malloc/free to pass arguments between functions is not efficient nor very elegant.
So the "static" option looked like the simplest & most elegant version.
Also, you forgot to update the comments in process_file, process_builtin, etc.
Indeed. v22 attached with better comments. -- Fabien.
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index cc80b3f..dd3fb1d 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -262,11 +262,13 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</> <variablelist> <varlistentry> - <term><option>-b</> <replaceable>scriptname</></term> - <term><option>--builtin</> <replaceable>scriptname</></term> + <term><option>-b</> <replaceable>scriptname[@weight]</></term> + <term><option>--builtin</>=<replaceable>scriptname[@weight]</></term> <listitem> <para> Add the specified builtin script to the list of executed scripts. + An optional integer weight after <literal>@</> allows to adjust the + probability of drawing the script. If not specified, it is set to 1. Available builtin scripts are: <literal>tpcb-like</>, <literal>simple-update</> and <literal>select-only</>. Unambiguous prefixes of builtin names are accepted. @@ -322,12 +324,14 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</> </varlistentry> <varlistentry> - <term><option>-f</> <replaceable>filename</></term> - <term><option>--file=</><replaceable>filename</></term> + <term><option>-f</> <replaceable>filename[@weight]</></term> + <term><option>--file=</><replaceable>filename[@weight]</></term> <listitem> <para> Add a transaction script read from <replaceable>filename</> to the list of executed scripts. + An optional integer weight after <literal>@</> allows to adjust the + probability of drawing the test. See below for details. </para> </listitem> @@ -687,9 +691,13 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</> <title>What is the <quote>Transaction</> Actually Performed in <application>pgbench</application>?</title> <para> - Pgbench executes test scripts chosen randomly from a specified list. + <application>pgbench</> executes test scripts chosen randomly + from a specified list. They include built-in scripts with <option>-b</> and user-provided custom scripts with <option>-f</>. + Each script may be given a relative weight specified after a + <literal>@</> so as to change its drawing probability. + The default weight is <literal>1</>. </para> <para> @@ -1194,12 +1202,11 @@ number of clients: 10 number of threads: 1 number of transactions per client: 1000 number of transactions actually processed: 10000/10000 +latency average = 15.844 ms +latency stddev = 2.715 ms tps = 618.764555 (including connections establishing) tps = 622.977698 (excluding connections establishing) -SQL script 1: <builtin: TPC-B (sort of)> - - 10000 transactions (100.0% of total, tps = 618.764555) - - latency average = 15.844 ms - - latency stddev = 2.715 ms +script statistics: - statement latencies in milliseconds: 0.004386 \set nbranches 1 * :scale 0.001343 \set ntellers 10 * :scale diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 8b0b17a..d7af86e 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -38,6 +38,7 @@ #include "portability/instr_time.h" #include <ctype.h> +#include <limits.h> #include <math.h> #include <signal.h> #include <sys/time.h> @@ -179,6 +180,8 @@ char *login = NULL; char *dbName; const char *progname; +#define WSEP '@' /* weight separator */ + volatile bool timer_exceeded = false; /* flag from signal handler */ /* variable definitions */ @@ -300,23 +303,27 @@ typedef struct static struct { const char *name; + int weight; Command **commands; StatsData stats; } sql_script[MAX_SCRIPTS]; /* SQL script files */ static int num_scripts; /* number of scripts in sql_script[] */ static int num_commands = 0; /* total number of Command structs */ +static int64 total_weight = 0; + static int debug = 0; /* debug flag */ /* Define builtin test scripts */ -#define N_BUILTIN 3 -static struct +typedef struct script_t { char *name; /* very short name for -b ... */ char *desc; /* short description */ - char *commands; /* actual pgbench script */ -} + char *script; /* actual pgbench script */ + Command **commands; /* temporary intermediate holder */ +} script_t; - builtin_script[] = +#define N_BUILTIN 3 +static script_t builtin_script[] = { { "tpcb-like", @@ -334,7 +341,8 @@ static struct "UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;\n" "UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;\n" "INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);\n" - "END;\n" + "END;\n", + NULL }, { "simple-update", @@ -350,14 +358,16 @@ static struct "UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;\n" "SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n" "INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);\n" - "END;\n" + "END;\n", + NULL }, { "select-only", "<builtin: select only>", "\\set naccounts " CppAsString2(naccounts) " * :scale\n" "\\setrandom aid 1 :naccounts\n" - "SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n" + "SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n", + NULL } }; @@ -392,9 +402,9 @@ usage(void) " --tablespace=TABLESPACE create tables in the specified tablespace\n" " --unlogged-tables create tables as unlogged tables\n" "\nOptions to select what to run:\n" - " -b, --builtin=NAME add buitin script (use \"-b list\" to display\n" - " available scripts)\n" - " -f, --file=FILENAME add transaction script from FILENAME\n" + " -b, --builtin=NAME[@W] add builtin script NAME weighted at W (default: 1)\n" + " (use \"-b list\" to list available scripts)\n" + " -f, --file=FILENAME[@W] add script FILENAME weighted at W (default: 1)\n" " -N, --skip-some-updates skip updates of pgbench_tellers and pgbench_branches\n" " (same as \"-b simple-update\")\n" " -S, --select-only perform SELECT-only transactions\n" @@ -1312,13 +1322,23 @@ clientDone(CState *st, bool ok) return false; /* always false */ } +/* return a script number with a weighted choice. */ static int chooseScript(TState *thread) { + int i = 0; + int64 w; + if (num_scripts == 1) return 0; - return getrand(thread, 0, num_scripts - 1); + w = getrand(thread, 0, total_weight - 1); + do + { + w -= sql_script[i++].weight; + } while (w >= 0); + + return i - 1; } /* return false iff client should be disconnected */ @@ -2620,15 +2640,17 @@ read_line_from_file(FILE *fd) } /* - * Given a file name, read it and return the array of Commands contained - * therein. "-" means to read stdin. + * Given a file name, read it and return a script_t with a description & + * an array of commands. "-" means to read stdin. */ -static Command ** +static script_t * process_file(char *filename) { #define COMMANDS_ALLOC_NUM 128 - Command **my_commands; + /* temporary holder to pass 2 information to addScript*/ + static script_t fs; + FILE *fd; int lineno, index; @@ -2636,7 +2658,8 @@ process_file(char *filename) int alloc_num; alloc_num = COMMANDS_ALLOC_NUM; - my_commands = (Command **) pg_malloc(sizeof(Command *) * alloc_num); + fs.commands = (Command **) pg_malloc(sizeof(Command *) * alloc_num); + fs.desc = filename; if (strcmp(filename, "-") == 0) fd = stdin; @@ -2644,7 +2667,7 @@ process_file(char *filename) { fprintf(stderr, "could not open file \"%s\": %s\n", filename, strerror(errno)); - pg_free(my_commands); + pg_free(fs.commands); return NULL; } @@ -2664,35 +2687,36 @@ process_file(char *filename) if (command == NULL) continue; - my_commands[index] = command; + fs.commands[index] = command; index++; if (index >= alloc_num) { alloc_num += COMMANDS_ALLOC_NUM; - my_commands = pg_realloc(my_commands, sizeof(Command *) * alloc_num); + fs.commands = pg_realloc(fs.commands, sizeof(Command *) * alloc_num); } } fclose(fd); - my_commands[index] = NULL; + fs.commands[index] = NULL; - return my_commands; + return &fs; } -static Command ** -process_builtin(const char *tb, const char *source) +/* process builtin script bi by adding the array of commands and returning it */ +static script_t * +process_builtin(script_t * bi) { #define COMMANDS_ALLOC_NUM 128 - Command **my_commands; int lineno, index; char buf[BUFSIZ]; int alloc_num; + char *tb = bi->script; alloc_num = COMMANDS_ALLOC_NUM; - my_commands = (Command **) pg_malloc(sizeof(Command *) * alloc_num); + bi->commands = (Command **) pg_malloc(sizeof(Command *) * alloc_num); lineno = 0; index = 0; @@ -2702,6 +2726,7 @@ process_builtin(const char *tb, const char *source) char *p; Command *command; + /* buffer overflow check? */ p = buf; while (*tb && *tb != '\n') *p++ = *tb++; @@ -2716,25 +2741,27 @@ process_builtin(const char *tb, const char *source) lineno += 1; - command = process_commands(buf, source, lineno); + command = process_commands(buf, bi->desc, lineno); if (command == NULL) continue; - my_commands[index] = command; + bi->commands[index] = command; index++; if (index >= alloc_num) { alloc_num += COMMANDS_ALLOC_NUM; - my_commands = pg_realloc(my_commands, sizeof(Command *) * alloc_num); + bi->commands = + pg_realloc(bi->commands, sizeof(Command *) * alloc_num); } } - my_commands[index] = NULL; + bi->commands[index] = NULL; - return my_commands; + return bi; } +/* show available builtin scripts */ static void listAvailableScripts(void) { @@ -2746,28 +2773,27 @@ listAvailableScripts(void) fprintf(stderr, "\n"); } -/* return builtin script "name" if unambiguous */ -static char * -findBuiltin(const char *name, char **desc) +/* return builtin script "name" if unambiguous, of fails if not found */ +static script_t * +findBuiltin(const char *name) { int i, found = 0, len = strlen(name); - char *commands = NULL; + script_t *result = NULL; for (i = 0; i < N_BUILTIN; i++) { if (strncmp(builtin_script[i].name, name, len) == 0) { - *desc = builtin_script[i].desc; - commands = builtin_script[i].commands; + result = & builtin_script[i]; found++; } } /* ok, unambiguous result */ if (found == 1) - return commands; + return result; /* error cases */ if (found == 0) @@ -2780,13 +2806,61 @@ findBuiltin(const char *name, char **desc) exit(1); } +/* + * Determine the weight specification from a script option (-b, -f), if any, + * and return it as an integer (1 is returned if there's no weight). The + * script name is returned in *script as a malloc'd string. + */ +static int +parseScriptWeight(const char *option, char **script) +{ + char *sep; + int weight; + + if ((sep = strrchr(option, WSEP))) + { + int namelen = sep - option; + long wtmp; + char *badp; + + /* generate the script name */ + *script = pg_malloc(namelen + 1); + strncpy(*script, option, namelen); + (*script)[namelen] = '\0'; + + /* process digits of the weight spec */ + errno = 0; + wtmp = strtol(sep + 1, &badp, 10); + if (errno != 0 || badp == sep + 1 || *badp != '\0') + { + fprintf(stderr, "invalid weight specification: %s\n", sep); + exit(1); + } + if (wtmp > INT_MAX || wtmp <= 0) + { + fprintf(stderr, + "weight specification out of range (1 .. %u): " INT64_FORMAT "\n", + INT_MAX, (int64) wtmp); + exit(1); + } + weight = wtmp; + } + else + { + *script = pg_strdup(option); + weight = 1; + } + + return weight; +} + +/* append a script to the list of scripts to process */ static void -addScript(const char *name, Command **commands) +addScript(script_t *script, int weight) { - if (commands == NULL || - commands[0] == NULL) + if (script->commands == NULL || script->commands[0] == NULL) { - fprintf(stderr, "empty command list for script \"%s\"\n", name); + fprintf(stderr, "empty command list for script \"%s\"\n", script->desc); exit(1); } @@ -2796,8 +2870,9 @@ addScript(const char *name, Command **commands) exit(1); } - sql_script[num_scripts].name = name; - sql_script[num_scripts].commands = commands; + sql_script[num_scripts].name = script->desc; + sql_script[num_scripts].weight = weight; + sql_script[num_scripts].commands = script->commands; initStats(&sql_script[num_scripts].stats, 0.0); num_scripts++; } @@ -2882,19 +2957,24 @@ printResults(TState *threads, StatsData *total, instr_time total_time, printf("tps = %f (including connections establishing)\n", tps_include); printf("tps = %f (excluding connections establishing)\n", tps_exclude); - /* Report per-command statistics */ - if (per_script_stats) + /* Report per-script/command statistics */ + if (per_script_stats || latency_limit || is_latencies) { int i; for (i = 0; i < num_scripts; i++) { - printf("SQL script %d: %s\n" - " - " INT64_FORMAT " transactions (%.1f%% of total, tps = %f)\n", - i + 1, sql_script[i].name, - sql_script[i].stats.cnt, - 100.0 * sql_script[i].stats.cnt / total->cnt, - sql_script[i].stats.cnt / time_include); + if (num_scripts > 1) + printf("SQL script %d: %s\n" + " - weight = %d\n" + " - " INT64_FORMAT " transactions (%.1f%% of total, tps = %f)\n", + i + 1, sql_script[i].name, + sql_script[i].weight, + sql_script[i].stats.cnt, + 100.0 * sql_script[i].stats.cnt / total->cnt, + sql_script[i].stats.cnt / time_include); + else + printf("script statistics:\n"); if (latency_limit) printf(" - number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n", @@ -2902,7 +2982,8 @@ printResults(TState *threads, StatsData *total, instr_time total_time, 100.0 * sql_script[i].stats.skipped / (sql_script[i].stats.skipped + sql_script[i].stats.cnt)); - printSimpleStats(" - latency", &sql_script[i].stats.latency); + if (num_scripts > 1) + printSimpleStats(" - latency", &sql_script[i].stats.latency); /* Report per-command latencies */ if (is_latencies) @@ -2985,7 +3066,7 @@ main(int argc, char **argv) instr_time conn_total_time; int64 latency_late = 0; StatsData stats; - char *desc; + int weight; int i; int nclients_dealt; @@ -3033,6 +3114,8 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "ih:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1) { + char *script; + switch (c) { case 'i': @@ -3164,27 +3247,25 @@ main(int argc, char **argv) exit(0); } - addScript(desc, - process_builtin(findBuiltin(optarg, &desc), desc)); + weight = parseScriptWeight(optarg, &script); + addScript(process_builtin(findBuiltin(script)), weight); benchmarking_option_set = true; internal_script_used = true; break; + case 'S': - addScript(desc, - process_builtin(findBuiltin("select-only", &desc), - desc)); + addScript(process_builtin(findBuiltin("select-only")), 1); benchmarking_option_set = true; internal_script_used = true; break; case 'N': - addScript(desc, - process_builtin(findBuiltin("simple-update", &desc), - desc)); + addScript(process_builtin(findBuiltin("simple-update")), 1); benchmarking_option_set = true; internal_script_used = true; break; case 'f': - addScript(optarg, process_file(optarg)); + weight = parseScriptWeight(optarg, &script); + addScript(process_file(script), weight); benchmarking_option_set = true; break; case 'D': @@ -3322,12 +3403,16 @@ main(int argc, char **argv) /* set default script if none */ if (num_scripts == 0 && !is_init_mode) { - addScript(desc, - process_builtin(findBuiltin("tpcb-like", &desc), desc)); + addScript(process_builtin(findBuiltin("tpcb-like")), 1); benchmarking_option_set = true; internal_script_used = true; } + /* compute total_weight */ + for (i = 0; i < num_scripts; i++) + /* cannot overflow: weight is 32b, total_weight 64b */ + total_weight += sql_script[i].weight; + /* show per script stats if several scripts are used */ if (num_scripts > 1) per_script_stats = true;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers