*-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: &lt;builtin: TPC-B (sort of)&gt;
- - 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

Reply via email to