pgbench - use enum for meta commands

Started by Fabien COELHOover 8 years ago8 messages
#1Fabien COELHO
coelho@cri.ensmp.fr
1 attachment(s)

Minor code enhancement.

While having a look at adding if/elif/else/endif to pgbench, and given the
current gset/cset added meta commands in cf queue, it occured to me that
repeated string comparisons to check for the various meta commands is
neither efficient nor readable. Use an enum instead, which are extensively
used already for other similar purposes.

--
Fabien.

Attachments:

pgbench-enum-meta-1.patchtext/x-diff; name=pgbench-enum-meta-1.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..3a88150 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -373,11 +373,21 @@ typedef enum QueryMode
 static QueryMode querymode = QUERY_SIMPLE;
 static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
 
+typedef enum MetaCommand
+{
+	META_NONE,
+	META_SET,
+	META_SETSHELL,
+	META_SHELL,
+	META_SLEEP
+} MetaCommand;
+
 typedef struct
 {
 	char	   *line;			/* text of command line */
 	int			command_num;	/* unique index of this Command struct */
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
+	MetaCommand meta;			/* which meta command, if appropriate */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
 	PgBenchExpr *expr;			/* parsed expression, if needed */
@@ -1721,6 +1731,26 @@ evaluateExpr(TState *thread, CState *st, PgBenchExpr *expr, PgBenchValue *retval
 	}
 }
 
+/* return meta-command enum identifier */
+static MetaCommand
+getMetaCommand(char * cmd)
+{
+	MetaCommand mc;
+	if (cmd == NULL)
+		mc = META_NONE;
+	else if (pg_strcasecmp(cmd, "set") == 0)
+		mc = META_SET;
+	else if (pg_strcasecmp(cmd, "setshell") == 0)
+		mc = META_SETSHELL;
+	else if (pg_strcasecmp(cmd, "shell") == 0)
+		mc = META_SHELL;
+	else if (pg_strcasecmp(cmd, "sleep") == 0)
+		mc = META_SLEEP;
+	else
+		mc = META_NONE;
+	return mc;
+}
+
 /*
  * Run a shell command. The result is assigned to the variable if not NULL.
  * Return true if succeeded, or false on error.
@@ -2218,7 +2248,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 						fprintf(stderr, "\n");
 					}
 
-					if (pg_strcasecmp(argv[0], "sleep") == 0)
+					if (command->meta == META_SLEEP)
 					{
 						/*
 						 * A \sleep doesn't execute anything, we just get the
@@ -2244,7 +2274,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					}
 					else
 					{
-						if (pg_strcasecmp(argv[0], "set") == 0)
+						if (command->meta == META_SET)
 						{
 							PgBenchExpr *expr = command->expr;
 							PgBenchValue result;
@@ -2263,7 +2293,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 								break;
 							}
 						}
-						else if (pg_strcasecmp(argv[0], "setshell") == 0)
+						else if (command->meta == META_SETSHELL)
 						{
 							bool		ret = runShellCommand(st, argv[1], argv + 2, argc - 2);
 
@@ -2283,7 +2313,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 								/* succeeded */
 							}
 						}
-						else if (pg_strcasecmp(argv[0], "shell") == 0)
+						else if (command->meta == META_SHELL)
 						{
 							bool		ret = runShellCommand(st, NULL, argv + 1, argc - 1);
 
@@ -3027,6 +3057,7 @@ process_sql_command(PQExpBuffer buf, const char *source)
 	my_command = (Command *) pg_malloc0(sizeof(Command));
 	my_command->command_num = num_commands++;
 	my_command->type = SQL_COMMAND;
+	my_command->meta = META_NONE;
 	initSimpleStats(&my_command->stats);
 
 	/*
@@ -3095,7 +3126,9 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 	my_command->argv[j++] = pg_strdup(word_buf.data);
 	my_command->argc++;
 
-	if (pg_strcasecmp(my_command->argv[0], "set") == 0)
+	my_command->meta = getMetaCommand(my_command->argv[0]);
+
+	if (my_command->meta == META_SET)
 	{
 		/* For \set, collect var name, then lex the expression. */
 		yyscan_t	yyscanner;
@@ -3150,7 +3183,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 												  expr_scanner_offset(sstate),
 												  true);
 
-	if (pg_strcasecmp(my_command->argv[0], "sleep") == 0)
+	if (my_command->meta == META_SLEEP)
 	{
 		if (my_command->argc < 2)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3191,13 +3224,13 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 							 my_command->argv[2], offsets[2] - start_offset);
 		}
 	}
-	else if (pg_strcasecmp(my_command->argv[0], "setshell") == 0)
+	else if (my_command->meta == META_SETSHELL)
 	{
 		if (my_command->argc < 3)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
 						 "missing argument", NULL, -1);
 	}
-	else if (pg_strcasecmp(my_command->argv[0], "shell") == 0)
+	else if (my_command->meta == META_SHELL)
 	{
 		if (my_command->argc < 2)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3205,6 +3238,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 	}
 	else
 	{
+		/* my_command->meta == META_NONE */
 		syntax_error(source, lineno, my_command->line, my_command->argv[0],
 					 "invalid command", NULL, -1);
 	}
#2Pavel Stehule
pavel.stehule@gmail.com
In reply to: Fabien COELHO (#1)
Re: pgbench - use enum for meta commands

2017-09-23 5:45 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Minor code enhancement.

While having a look at adding if/elif/else/endif to pgbench, and given the
current gset/cset added meta commands in cf queue, it occured to me that
repeated string comparisons to check for the various meta commands is
neither efficient nor readable. Use an enum instead, which are extensively
used already for other similar purposes.

+1

Pavel

Show quoted text

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Aleksandr Parfenov
a.parfenov@postgrespro.ru
In reply to: Fabien COELHO (#1)
Re: pgbench - use enum for meta commands

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi,

Looks good to me.

The only thing I'm not quite sure about is a comment "which meta command ...".
Maybe it's better to write it without question word, something like "meta command identifier..."?

The new status of this patch is: Ready for Committer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Aleksandr Parfenov (#3)
Re: pgbench - use enum for meta commands

The only thing I'm not quite sure about is a comment "which meta command
...". Maybe it's better to write it without question word, something
like "meta command identifier..."?

Ok. I agree.

Updated version attached. I also added a const on a function parameter.

Just a note about the motivation: I want to add the same "\if" syntax
added to psql, but it requires to look at the meta command in a number of
places to manage the automaton status, and the strcmp solution looked both
ugly and inefficient. So this small refactoring is just a preliminary to
the "\if" patch, some day, after this one get committed, if it gets
committed.

The new status of this patch is: Ready for Committer

Thanks for the review.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#4)
1 attachment(s)
Re: pgbench - use enum for meta commands

Updated version attached.

<sigh>. Here is the patch. Sorry for the noise.

--
Fabien.

Attachments:

pgbench-enum-meta-2.patchtext/x-diff; name=pgbench-enum-meta-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 5d8a01c..6bd3e52 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -373,11 +373,21 @@ typedef enum QueryMode
 static QueryMode querymode = QUERY_SIMPLE;
 static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
 
+typedef enum MetaCommand
+{
+	META_NONE,
+	META_SET,
+	META_SETSHELL,
+	META_SHELL,
+	META_SLEEP
+} MetaCommand;
+
 typedef struct
 {
 	char	   *line;			/* text of command line */
 	int			command_num;	/* unique index of this Command struct */
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
+	MetaCommand meta;			/* meta command identifier, if appropriate */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
 	PgBenchExpr *expr;			/* parsed expression, if needed */
@@ -1721,6 +1731,26 @@ evaluateExpr(TState *thread, CState *st, PgBenchExpr *expr, PgBenchValue *retval
 	}
 }
 
+/* return meta-command enum identifier */
+static MetaCommand
+getMetaCommand(const char * cmd)
+{
+	MetaCommand mc;
+	if (cmd == NULL)
+		mc = META_NONE;
+	else if (pg_strcasecmp(cmd, "set") == 0)
+		mc = META_SET;
+	else if (pg_strcasecmp(cmd, "setshell") == 0)
+		mc = META_SETSHELL;
+	else if (pg_strcasecmp(cmd, "shell") == 0)
+		mc = META_SHELL;
+	else if (pg_strcasecmp(cmd, "sleep") == 0)
+		mc = META_SLEEP;
+	else
+		mc = META_NONE;
+	return mc;
+}
+
 /*
  * Run a shell command. The result is assigned to the variable if not NULL.
  * Return true if succeeded, or false on error.
@@ -2214,7 +2244,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 						fprintf(stderr, "\n");
 					}
 
-					if (pg_strcasecmp(argv[0], "sleep") == 0)
+					if (command->meta == META_SLEEP)
 					{
 						/*
 						 * A \sleep doesn't execute anything, we just get the
@@ -2240,7 +2270,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					}
 					else
 					{
-						if (pg_strcasecmp(argv[0], "set") == 0)
+						if (command->meta == META_SET)
 						{
 							PgBenchExpr *expr = command->expr;
 							PgBenchValue result;
@@ -2259,7 +2289,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 								break;
 							}
 						}
-						else if (pg_strcasecmp(argv[0], "setshell") == 0)
+						else if (command->meta == META_SETSHELL)
 						{
 							bool		ret = runShellCommand(st, argv[1], argv + 2, argc - 2);
 
@@ -2279,7 +2309,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 								/* succeeded */
 							}
 						}
-						else if (pg_strcasecmp(argv[0], "shell") == 0)
+						else if (command->meta == META_SHELL)
 						{
 							bool		ret = runShellCommand(st, NULL, argv + 1, argc - 1);
 
@@ -3023,6 +3053,7 @@ process_sql_command(PQExpBuffer buf, const char *source)
 	my_command = (Command *) pg_malloc0(sizeof(Command));
 	my_command->command_num = num_commands++;
 	my_command->type = SQL_COMMAND;
+	my_command->meta = META_NONE;
 	initSimpleStats(&my_command->stats);
 
 	/*
@@ -3091,7 +3122,9 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 	my_command->argv[j++] = pg_strdup(word_buf.data);
 	my_command->argc++;
 
-	if (pg_strcasecmp(my_command->argv[0], "set") == 0)
+	my_command->meta = getMetaCommand(my_command->argv[0]);
+
+	if (my_command->meta == META_SET)
 	{
 		/* For \set, collect var name, then lex the expression. */
 		yyscan_t	yyscanner;
@@ -3146,7 +3179,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 												  expr_scanner_offset(sstate),
 												  true);
 
-	if (pg_strcasecmp(my_command->argv[0], "sleep") == 0)
+	if (my_command->meta == META_SLEEP)
 	{
 		if (my_command->argc < 2)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3187,13 +3220,13 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 							 my_command->argv[2], offsets[2] - start_offset);
 		}
 	}
-	else if (pg_strcasecmp(my_command->argv[0], "setshell") == 0)
+	else if (my_command->meta == META_SETSHELL)
 	{
 		if (my_command->argc < 3)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
 						 "missing argument", NULL, -1);
 	}
-	else if (pg_strcasecmp(my_command->argv[0], "shell") == 0)
+	else if (my_command->meta == META_SHELL)
 	{
 		if (my_command->argc < 2)
 			syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3201,6 +3234,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
 	}
 	else
 	{
+		/* my_command->meta == META_NONE */
 		syntax_error(source, lineno, my_command->line, my_command->argv[0],
 					 "invalid command", NULL, -1);
 	}
#6Noname
a.parfenov@postgrespro.ru
In reply to: Fabien COELHO (#5)
Re: pgbench - use enum for meta commands

On Thu, 2 Nov 2017 17:50:52 +0100 (CET)
Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Updated version attached.

<sigh>. Here is the patch. Sorry for the noise.

Everything alright. Patch is ready for commiter.

--
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Fabien COELHO (#5)
Re: pgbench - use enum for meta commands

Fabien COELHO <coelho@cri.ensmp.fr> writes:

[ pgbench-enum-meta-2.patch ]

Pushed with some trivial cosmetic adjustments (pgindent changed
it more than I did).

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Tom Lane (#7)
Re: pgbench - use enum for meta commands

[ pgbench-enum-meta-2.patch ]

Pushed with some trivial cosmetic adjustments (pgindent changed
it more than I did).

Ok. Thanks.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers