Hello Amit,
Custom script looks like: \; select a \into a from tab where a = 1; \set i debug(:a) I get the following error: undefined variable "a" client 0 aborted in state 1; execution of meta-command failed
Good catch!Indeed, there is a problem with empty commands which are simply ignored by libpq/postgres if there are other commands around, so my synchronization between results & commands was too naive.
In order to fix this, I made the scanner also count empty commands and ignore comments. I guessed that proposing to change libpq/postgres behavior was not an option.
Comments on the patch follow: + <listitem> + <para> + Stores the first fields of the resulting row from the current or preceding + <command>SELECT</> command into these variables. Any command returning rows ought to work, no?
Yes. I put "SQL command" instead.
- char *line; /* text of command line */ + char *line; /* first line for short display */ + char *lines; /* full multi-line text of command */ When I looked at this and related hunks (and also the hunks related to semicolons), it made me wonder whether this patch contains two logical changes. Is this a just a refactoring for the \into implementation or does this provide some new externally visible feature?
There is essentially a refactoring that I did when updating how Command is managed because it has to be done in several stages to fit "into" into it and to take care of compounds.
However there was small "new externally visible feature": on -r, instead of cutting abruptly a multiline command at the end of the first line it appended "..." as an ellipsis because it looked nicer.
I've removed this small visible changed.
char *argv[MAX_ARGS]; /* command word list */
+ int compound; /* last compound command (number of \;) */
+ char ***intos; /* per-compound command \into variables */
Need an extra space for intos to align with earlier fields.
Ok.
Also I wonder if intonames or intoargs would be a slightly better name for the field.
I put "intovars" as they are variable names.
+static bool +read_response(CState *st, char ** intos[]) Usual style seems to be to use ***intos here.
Ok.
+ fprintf(stderr, + "client %d state %d compound %d: " + "cannot apply \\into to non SELECT statement\n", + st->id, st->state, compound); How about make this error message say something like other messages related to \into, perhaps something like: "\\into cannot follow non SELECT statements\n"
As you pointed out above, there may be statements without "SELECT" which which return a row. I wrote "\\into expects a row" instead.
/*
* Read and discard the query result; note this is not included in
- * the statement latency numbers.
+ * the statement latency numbers (above), thus if reading the
+ * response fails the transaction is counted nevertheless.
*/
Does this comment need to mention that the result is not discarded when
\into is specified?
Hmmm. The result structure is discarded, but the results are copied into variables before that, so the comments seems ok...
+ my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1)); Need space: s/char**/char **/g
Ok.
This comments applies also to a couple of nearby places.
Indeed.
- my_command->line = pg_malloc(nlpos - p + 1); + my_command->line = pg_malloc(nlpos - p + 1 + 3); A comment mentioning what the above means would be helpful.
Ok. I removed the "+ 3" anyway.
+ bool sql_command_in_progress = false; This variable's name could be slightly confusing to readers. If I understand its purpose correctly, perhaps it could be called sql_command_continues.
It is possible. I like 'in progress' though. Why is it confusing?It means that the current command is not finished yet and more is expected, that is the final ';' has not been encountered.
+ if (index == 0) + syntax_error(desc, lineno, NULL, NULL, + "\\into cannot start a script", + NULL, -1); How about: "\\into cannot be at the beginning of a script" ?
It is possible, but it's quite longer... I'm not a native speaker, so I'm do not know whether it would be better.
The attached patch takes into all your comments but: - comment about discarded results... - the sql_command_in_progress variable name change - the longer message on into at the start of a script -- Fabien.
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 285608d..0a474e1 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -809,6 +809,30 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
</para>
<variablelist>
+ <varlistentry id='pgbench-metacommand-into'>
+ <term>
+ <literal>\into <replaceable>var1</> [<replaceable>var2</> ...]</literal>
+ </term>
+
+ <listitem>
+ <para>
+ Stores the first fields of the resulting row from the current or preceding
+ SQL command into these variables.
+ The queries must yield exactly one row and the number of provided
+ variables must be less than the total number of columns of the results.
+ This meta command does not end the current SQL command.
+ </para>
+
+ <para>
+ Example:
+<programlisting>
+SELECT abalance \into abalance
+ FROM pgbench_accounts WHERE aid=5432;
+</programlisting>
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id='pgbench-metacommand-set'>
<term>
<literal>\set <replaceable>varname</> <replaceable>expression</></literal>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 56c37d5..42a9d2c 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -302,11 +302,14 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
typedef struct
{
- char *line; /* text of command line */
+ char *line; /* first line for short display */
+ char *lines; /* full multi-line text of command */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
+ int compound; /* last compound command (number of \;) */
+ char ***intovars; /* per-compound command \into variables */
PgBenchExpr *expr; /* parsed expression, if needed */
SimpleStats stats; /* time spent in this command */
} Command;
@@ -1150,6 +1153,107 @@ getQueryParams(CState *st, const Command *command, const char **params)
params[i] = getVariable(st, command->argv[i + 1]);
}
+/* read all responses from backend */
+static bool
+read_response(CState *st, char ***intovars)
+{
+ PGresult *res;
+ int compound = -1;
+
+ while ((res = PQgetResult(st->con)) != NULL)
+ {
+ compound += 1;
+
+ switch (PQresultStatus(res))
+ {
+ case PGRES_COMMAND_OK: /* non-SELECT commands */
+ case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
+ if (intovars[compound] != NULL)
+ {
+ fprintf(stderr,
+ "client %d state %d compound %d: "
+ "\\into expects a row\n",
+ st->id, st->state, compound);
+ st->ecnt++;
+ return false;
+ }
+ break; /* OK */
+
+ case PGRES_TUPLES_OK:
+ if (intovars[compound] != NULL)
+ {
+ /* store result into variables */
+ int ntuples = PQntuples(res),
+ nfields = PQnfields(res),
+ f = 0;
+
+ if (ntuples != 1)
+ {
+ fprintf(stderr,
+ "client %d state %d compound %d: "
+ "expecting one row, got %d\n",
+ st->id, st->state, compound, ntuples);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ while (intovars[compound][f] != NULL && f < nfields)
+ {
+ /* store result as a string */
+ if (!putVariable(st, "into", intovars[compound][f],
+ PQgetvalue(res, 0, f)))
+ {
+ /* internal error, should it rather abort? */
+ fprintf(stderr,
+ "client %d state %d compound %d: "
+ "error storing into var %s\n",
+ st->id, st->state, compound, intovars[compound][f]);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ f++;
+ }
+
+ if (intovars[compound][f] != NULL)
+ {
+ fprintf(stderr,
+ "client %d state %d compound %d: missing results"
+ " to fill into variable %s\n",
+ st->id, st->state, compound, intovars[compound][f]);
+ st->ecnt++;
+ return false;
+ }
+ }
+ break; /* OK */
+
+ default:
+ /* everything else is unexpected, so probably an error */
+ fprintf(stderr, "client %d aborted in state %d compound %d: %s",
+ st->id, st->state, compound, PQerrorMessage(st->con));
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ PQclear(res);
+ }
+
+ if (compound == -1)
+ {
+ fprintf(stderr, "client %d state %d: no results\n", st->id, st->state);
+ st->ecnt++;
+ return false;
+ }
+
+ return true;
+}
+
/* get a value as an int, tell if there is a problem */
static bool
coerceToInt(PgBenchValue *pval, int64 *ival)
@@ -1766,7 +1870,6 @@ chooseScript(TState *thread)
static bool
doCustom(TState *thread, CState *st, StatsData *agg)
{
- PGresult *res;
Command **commands;
bool trans_needs_throttle = false;
instr_time now;
@@ -1893,23 +1996,11 @@ top:
{
/*
* Read and discard the query result; note this is not included in
- * the statement latency numbers.
+ * the statement latency numbers (above), thus if reading the
+ * response fails the transaction is counted nevertheless.
*/
- res = PQgetResult(st->con);
- switch (PQresultStatus(res))
- {
- case PGRES_COMMAND_OK:
- case PGRES_TUPLES_OK:
- case PGRES_EMPTY_QUERY:
- break; /* OK */
- default:
- fprintf(stderr, "client %d aborted in state %d: %s",
- st->id, st->state, PQerrorMessage(st->con));
- PQclear(res);
- return clientDone(st);
- }
- PQclear(res);
- discard_response(st);
+ if (!read_response(st, commands[st->state]->intovars))
+ return clientDone(st);
}
if (commands[st->state + 1] == NULL)
@@ -2704,22 +2795,10 @@ syntax_error(const char *source, int lineno,
exit(1);
}
-/*
- * Parse a SQL command; return a Command struct, or NULL if it's a comment
- *
- * On entry, psqlscan.l has collected the command into "buf", so we don't
- * really need to do much here except check for comment and set up a
- * Command struct.
- */
-static Command *
-process_sql_command(PQExpBuffer buf, const char *source)
+static char *
+skip_sql_comments(char *p)
{
- Command *my_command;
- char *p;
- char *nlpos;
-
/* Skip any leading whitespace, as well as "--" style comments */
- p = buf->data;
for (;;)
{
if (isspace((unsigned char) *p))
@@ -2739,17 +2818,85 @@ process_sql_command(PQExpBuffer buf, const char *source)
if (*p == '\0')
return NULL;
+ return p;
+}
+
+/*
+ * Parse a SQL command; return a Command struct, or NULL if it's a comment
+ *
+ * On entry, psqlscan.l has collected the command into "buf", so we don't
+ * really need to do much here except check for comment and set up a
+ * Command struct.
+ */
+static Command *
+create_sql_command(PQExpBuffer buf, const char *source, int compounds)
+{
+ Command *my_command;
+ char *p = skip_sql_comments(buf->data);
+
+ if (p == NULL)
+ return NULL;
+
/* Allocate and initialize Command structure */
my_command = (Command *) pg_malloc0(sizeof(Command));
my_command->command_num = num_commands++;
my_command->type = SQL_COMMAND;
my_command->argc = 0;
+ my_command->compound = compounds;
+ my_command->intovars = pg_malloc0(sizeof(char **) * (compounds+1));
initSimpleStats(&my_command->stats);
+ my_command->lines = pg_strdup(p);
+
+ return my_command;
+}
+
+static void
+append_sql_command(Command *my_command, char *more, int compounds)
+{
+ size_t lmore;
+ size_t len = strlen(my_command->lines);
+ int space;
+
+ Assert(my_command->type == SQL_COMMAND && len > 0);
+
+ more = skip_sql_comments(more);
+
+ if (more == NULL)
+ return;
+
+ /* add a separator if needed */
+ space = isspace(my_command->lines[len-1]) ? 0 : 1;
+ lmore = strlen(more);
+ my_command->lines = pg_realloc(my_command->lines, len + space + lmore + 1);
+ if (space > 0)
+ my_command->lines[len] = '\n';
+ memcpy(my_command->lines + len + space, more, lmore+1);
+
+ if (compounds > 0)
+ {
+ int nc = my_command->compound + compounds;
+ my_command->intovars =
+ pg_realloc(my_command->intovars, sizeof(char **) * (nc+1));
+ memset(my_command->intovars + my_command->compound + 1, 0,
+ sizeof(char **) * compounds);
+ my_command->compound = nc;
+ }
+}
+
+static void
+postprocess_sql_command(Command *my_command)
+{
+ char *nlpos;
+ char *p;
+
+ Assert(my_command->type == SQL_COMMAND);
+
/*
* If SQL command is multi-line, we only want to save the first line as
* the "line" label.
*/
+ p = my_command->lines;
nlpos = strchr(p, '\n');
if (nlpos)
{
@@ -2763,19 +2910,17 @@ process_sql_command(PQExpBuffer buf, const char *source)
switch (querymode)
{
case QUERY_SIMPLE:
- my_command->argv[0] = pg_strdup(p);
+ my_command->argv[0] = my_command->lines;
my_command->argc++;
break;
case QUERY_EXTENDED:
case QUERY_PREPARED:
- if (!parseQuery(my_command, p))
+ if (!parseQuery(my_command, my_command->lines))
exit(1);
break;
default:
exit(1);
}
-
- return my_command;
}
/*
@@ -2933,6 +3078,13 @@ process_backslash_command(PsqlScanState sstate, const char *source)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
"missing command", NULL, -1);
}
+ else if (pg_strcasecmp(my_command->argv[0], "into") == 0)
+ {
+ /* at least one variable name must be provided */
+ if (my_command->argc < 2)
+ syntax_error(source, lineno, my_command->line, my_command->argv[0],
+ "missing variable names", NULL, -1);
+ }
else
{
syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -2944,6 +3096,15 @@ process_backslash_command(PsqlScanState sstate, const char *source)
return my_command;
}
+static bool
+ends_with_semicolon(const char *buf)
+{
+ int i = strlen(buf)-1;
+ while (i > 0 && isspace(buf[i]))
+ i--;
+ return i > 0 && buf[i] == ';';
+}
+
/*
* Parse a script (either the contents of a file, or a built-in script)
* and add it to the list of scripts.
@@ -2956,6 +3117,9 @@ ParseScript(const char *script, const char *desc, int weight)
PQExpBufferData line_buf;
int alloc_num;
int index;
+ bool sql_command_in_progress = false;
+ int lineno;
+ int start_offset;
#define COMMANDS_ALLOC_NUM 128
alloc_num = COMMANDS_ALLOC_NUM;
@@ -2968,6 +3132,7 @@ ParseScript(const char *script, const char *desc, int weight)
/* Prepare to parse script */
sstate = psql_scan_create(&pgbench_callbacks);
+ sstate->rm_c_comments = true;
/*
* Ideally, we'd scan scripts using the encoding and stdstrings settings
@@ -2979,6 +3144,7 @@ ParseScript(const char *script, const char *desc, int weight)
* stdstrings should be true, which is a bit riskier.
*/
psql_scan_setup(sstate, script, strlen(script), 0, true);
+ start_offset = expr_scanner_offset(sstate) - 1;
initPQExpBuffer(&line_buf);
@@ -2988,31 +3154,32 @@ ParseScript(const char *script, const char *desc, int weight)
{
PsqlScanResult sr;
promptStatus_t prompt;
- Command *command;
+ Command *command = NULL;
resetPQExpBuffer(&line_buf);
+ lineno = expr_scanner_get_lineno(sstate, start_offset);
+
+ sstate->semicolons = 0;
+ sstate->empty_cmds = 0;
+ sstate->last_cmd_start = 0;
sr = psql_scan(sstate, &line_buf, &prompt);
- /* If we collected a SQL command, process that */
- command = process_sql_command(&line_buf, desc);
- if (command)
+ if (sql_command_in_progress)
{
- ps.commands[index] = command;
- index++;
-
- if (index >= alloc_num)
- {
- alloc_num += COMMANDS_ALLOC_NUM;
- ps.commands = (Command **)
- pg_realloc(ps.commands, sizeof(Command *) * alloc_num);
- }
+ /* a multi-line command was interrupted by an \into */
+ append_sql_command(ps.commands[index-1], line_buf.data,
+ sstate->semicolons - sstate->empty_cmds);
+ sql_command_in_progress = false;
}
-
- /* If we reached a backslash, process that */
- if (sr == PSCAN_BACKSLASH)
+ else
{
- command = process_backslash_command(sstate, desc);
+ /* If we collected a new SQL command, process that */
+ command =
+ create_sql_command(&line_buf, desc,
+ sstate->semicolons - sstate->empty_cmds);
+
+ /* store new command */
if (command)
{
ps.commands[index] = command;
@@ -3027,6 +3194,75 @@ ParseScript(const char *script, const char *desc, int weight)
}
}
+ if (sr == PSCAN_BACKSLASH)
+ {
+ command = process_backslash_command(sstate, desc);
+
+ if (command)
+ {
+ /* Merge \into into preceeding SQL command ... */
+ if (pg_strcasecmp(command->argv[0], "into") == 0)
+ {
+ int cindex, i;
+ Command *into_cmd;
+
+ if (index == 0)
+ syntax_error(desc, lineno, NULL, NULL,
+ "\\into cannot start a script",
+ NULL, -1);
+
+ into_cmd = ps.commands[index-1];
+
+ if (into_cmd->type != SQL_COMMAND)
+ syntax_error(desc, lineno, NULL, NULL,
+ "\\into must follow an SQL command",
+ into_cmd->line, -1);
+
+ /* this \into applies to this sub-command */
+ cindex = into_cmd->compound -
+ (ends_with_semicolon(line_buf.data) ? 1 : 0);
+
+ if (into_cmd->intovars[cindex] != NULL)
+ syntax_error(desc, lineno, NULL, NULL,
+ "\\into cannot follow an \\into",
+ NULL, -1);
+
+ /* check that all variable names are valid */
+ for (i = 1; i < command->argc; i++)
+ {
+ if (!isLegalVariableName(command->argv[i]))
+ syntax_error(desc, lineno, NULL, NULL,
+ "\\into invalid variable name",
+ command->argv[i], -1);
+ }
+
+ into_cmd->intovars[cindex] =
+ pg_malloc0(sizeof(char *) * (command->argc+1));
+
+ memcpy(into_cmd->intovars[cindex], command->argv + 1,
+ sizeof(char*) * command->argc);
+
+ /* cleanup unused backslash command */
+ pg_free(command);
+
+ /* "SELECT ... \into ..." follow-up */
+ sql_command_in_progress = *line_buf.data != '\0';
+ }
+ else /* any other backslash command is a Command */
+ {
+ ps.commands[index] = command;
+ index++;
+
+ if (index >= alloc_num)
+ {
+ alloc_num += COMMANDS_ALLOC_NUM;
+ ps.commands = (Command **)
+ pg_realloc(ps.commands, sizeof(Command *) * alloc_num);
+ }
+ }
+ }
+ }
+
/* Done if we reached EOF */
if (sr == PSCAN_INCOMPLETE || sr == PSCAN_EOL)
break;
@@ -3034,6 +3270,11 @@ ParseScript(const char *script, const char *desc, int weight)
ps.commands[index] = NULL;
+ /* complete SQL command initializations */
+ while (--index >= 0)
+ if (ps.commands[index]->type == SQL_COMMAND)
+ postprocess_sql_command(ps.commands[index]);
+
addScript(ps);
termPQExpBuffer(&line_buf);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index ab0f822..4d28fdb 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -11,6 +11,7 @@
#ifndef PGBENCH_H
#define PGBENCH_H
+#include "fe_utils/psqlscan_int.h"
#include "fe_utils/psqlscan.h"
/*
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 55067b4..aa1ab6d 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -71,6 +71,8 @@ typedef int YYSTYPE;
extern int psql_yyget_column(yyscan_t yyscanner);
extern void psql_yyset_column(int column_no, yyscan_t yyscanner);
+static bool buffer_empty_since(PQExpBuffer buf, size_t len);
+
%}
%option reentrant
@@ -388,14 +390,16 @@ other .
BEGIN(xc);
/* Put back any characters past slash-star; see above */
yyless(2);
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
<xc>{xcstart} {
cur_state->xcdepth++;
/* Put back any characters past slash-star; see above */
yyless(2);
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
<xc>{xcstop} {
@@ -403,19 +407,23 @@ other .
BEGIN(INITIAL);
else
cur_state->xcdepth--;
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
<xc>{xcinside} {
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
<xc>{op_chars} {
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
<xc>\*+ {
- ECHO;
+ if (!cur_state->rm_c_comments)
+ ECHO;
}
{xbstart} {
@@ -680,6 +688,16 @@ other .
"\\"[;:] {
/* Force a semicolon or colon into the query buffer */
+ if (yytext[1] == ';')
+ {
+ /* count compound commands */
+ cur_state->semicolons++;
+ /* detect empty commands */
+ if (buffer_empty_since(cur_state->output_buf, cur_state->last_cmd_start))
+ cur_state->empty_cmds++;
+ cur_state->last_cmd_start =
+ cur_state->output_buf->len + 2;
+ }
psqlscan_emit(cur_state, yytext + 1, 1);
}
@@ -1323,6 +1341,20 @@ psqlscan_prepare_buffer(PsqlScanState state, const char *txt, int len,
return yy_scan_buffer(newtxt, len + 2, state->scanner);
}
+/* tell whether there were only spaces inserted since len
+ */
+static bool
+buffer_empty_since(PQExpBuffer buf, size_t len)
+{
+ while (len < buf->len)
+ {
+ if (!isspace(buf->data[len]))
+ return false;
+ len++;
+ }
+ return true;
+}
+
/*
* psqlscan_emit() --- body for ECHO macro
*
diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h
index a52929d..9ec5f51 100644
--- a/src/include/fe_utils/psqlscan_int.h
+++ b/src/include/fe_utils/psqlscan_int.h
@@ -112,6 +112,10 @@ typedef struct PsqlScanStateData
int start_state; /* yylex's starting/finishing state */
int paren_depth; /* depth of nesting in parentheses */
int xcdepth; /* depth of nesting in slash-star comments */
+ int semicolons; /* number of embedded (\;) semi-colons */
+ int empty_cmds; /* number of empty commands (\;\;) */
+ size_t last_cmd_start; /* position in output buffer */
+ bool rm_c_comments; /* whether to remove C comments */
char *dolqstart; /* current $foo$ quote start string */
/*
into-8.sql
Description: application/sql
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
