Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread Fabien COELHO



[ 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


Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread Tom Lane
Fabien COELHO  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


Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread a . parfenov

On Thu, 2 Nov 2017 17:50:52 +0100 (CET)
Fabien COELHO  wrote:


> Updated version attached.

. 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


Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread Fabien COELHO



Updated version attached.


. Here is the patch. Sorry for the noise.

--
Fabien.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);
 	}

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


Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread Fabien COELHO


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


Re: [HACKERS] pgbench - use enum for meta commands

2017-11-02 Thread Aleksandr Parfenov
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


Re: [HACKERS] pgbench - use enum for meta commands

2017-09-22 Thread Pavel Stehule
2017-09-23 5:45 GMT+02:00 Fabien COELHO :

>
> 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


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