Re: [HACKERS] pgbench - allow to store select results into variables
Hello Pavel, Here is a v13, which is just a rebase after the documentation xml-ization. Here is a v14, after yet another rebase, and some comments added to answer your new comments. I am looking to this patch. Not sure if "cset" is best name - maybe "eset" .. like embeded set? I used c for "compound", because they compound SQL commands. Now I do not have a very strong opinion, only that it should be some letter which some logic followed by "set". The variables and fields in the source currently use "compound" everywhere, if this is changed they should be updated accordingly. ISTM that the ";" is embedded, but the commands are compound, so "compound" seems better word to me. However it is debatable. If there some standard naming for the concept, it should be used. The code of append_sql_command is not too readable and is not enough commented. Ok. I have added comments in the code. I don't understand why you pass a param compounds to append_sql_command, when this value is stored in my_command->compound from create_sql_command? This is the number of compound commands in the "more" string. It must be updated as well as the command text, so that the my_command text and number of compounds is consistant. Think of one initialization followed by two appends: SELECT 1 AS x \cset SELECT 2 \; SELECT 3 AS y \cset SELECT 4 \; SELECT 5 \; SELECT 6 AS z \gset In the end, we must have the full 6 queries "SELECT 1 AS x \; SELECT 2 \; SELECT 3 AS y \; SELECT 4 \; SELECT 5 \; SELECT 6 AS z" and know that we want to set variables from queries 1, 3 and 6 and ignore the 3 others. Or maybe some unhappy field or variable names was chosen. It seems ok to me. What would your suggest? -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index e509e6c..44e8896 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -818,6 +818,51 @@ pgbench options d + + + \cset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \cset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and + p_three with integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \cset +SELECT 3 AS three \gset p_ + + + + + +\cset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index d4a6035..32262df 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -384,12 +384,15 @@ 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) */ MetaCommand meta; /* meta command identifier, or META_NONE */ int argc; /* number of command words */ char *argv[MAX_ARGS]; /* command word list */ + int compound; /* last compound command (number of \;) */ + char **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1264,6 +1267,104 @@ 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 **gset) +{ + PGresult *res; + int compound = 0; + + while ((res = PQgetResult(st->con)) != NULL) + { + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ +if (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store
Re: [HACKERS] pgbench - allow to store select results into variables
Hi 2017-10-20 18:37 GMT+02:00 Fabien COELHO: > > Here is a v12. >> > > Here is a v13, which is just a rebase after the documentation xml-ization. > I am looking to this patch. Not sure if "cset" is best name - maybe "eset" .. like embeded set? The code of append_sql_command is not too readable and is not enough commented. I don't understand why you pass a param compounds to append_sql_command, when this value is stored in my_command->compound from create_sql_command? Or maybe some unhappy field or variable names was chosen. Regards Pavel > > -- > Fabien.
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v12. Here is a v13, which is just a rebase after the documentation xml-ization. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index e509e6c..44e8896 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -818,6 +818,51 @@ pgbench options d + + + \cset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \cset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and + p_three with integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \cset +SELECT 3 AS three \gset p_ + + + + + +\cset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 5d8a01c..37ed07b 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1254,6 +1257,104 @@ 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 **gset) +{ + PGresult *res; + int compound = 0; + + while ((res = PQgetResult(st->con)) != NULL) + { + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ +if (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, + varname); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + if (*gset[compound] != '\0') + free(varname); + } +} +break; /* OK */ + + default: +/* everything else is unexpected, so probably an error */ +fprintf(stderr, + "client %d file %d aborted in command %d compound %d: %s", + st->id, st->use_file, st->command, compound, + PQerrorMessage(st->con)); +st->ecnt++; +PQclear(res); +discard_response(st); +return false; + } + + PQclear(res); + compound += 1; + } + + if (compound == 0) + { + fprintf(stderr, "client %d command %d: no results\n", st->id, st->command); + st->ecnt++; + return false; + } + + return
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v12. There is no changes in the code or documentation, only TAP tests are added. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index f5db8d1..9ad82d4 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -818,6 +818,51 @@ pgbench options dbname + + + \cset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \cset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \cset +SELECT 3 AS three \gset p_ + + + + + +\cset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index e37496c..454127c 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1254,6 +1257,104 @@ 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 **gset) +{ + PGresult *res; + int compound = 0; + + while ((res = PQgetResult(st->con)) != NULL) + { + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ +if (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, + varname); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + if (*gset[compound] != '\0') + free(varname); + } +} +break; /* OK */ + + default: +/* everything else is unexpected, so probably an error */ +fprintf(stderr, + "client %d file %d aborted in command %d compound %d: %s", + st->id, st->use_file, st->command, compound, + PQerrorMessage(st->con)); +st->ecnt++; +PQclear(res); +discard_response(st); +return false; + } + + PQclear(res); + compound += 1; + } + + if (compound == 0) + { + fprintf(stderr, "client %d command %d: no results\n", st->id, st->command); + st->ecnt++; + return false; + } + +
Re: [HACKERS] pgbench - allow to store select results into variables
Looks good to me. I have marked the patch status as "Ready for committer". LGTM too. Pushed with a minor adjustment to make parseVariable() have exactly the same test as valid_variable_name(). \set ありがとうございました 1 \set 谢谢 2 \set dankeschön 3 :-) -- 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 - allow to store select results into variables
Tatsuo Ishiiwrites: >> Here is a v3 with a more precise comment. > Looks good to me. I have marked the patch status as "Ready for > committer". LGTM too. Pushed with a minor adjustment to make parseVariable() have exactly the same test as valid_variable_name(). 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 - allow to store select results into variables
Hi 2017-08-13 20:33 GMT+02:00 Fabien COELHO: > > Here is a v11. > > It is basically a simple rebase after Tom committed the "pgbench -M order" > patch. It interfered because the compound command management also needs > to delay part of the SQL command initialization. Some patch are luckier > than others:-) > > Here is a v10: >> >> - does not talk about ASCII variable name constraint, as a patch has been >> submitted independently to lift this constraint. >> >> - rename gcset to cset (compound set, \; + \set), where gset is ; + \set, >> because "\gcset" looked really strange. >> >> - simplify the code a little bit. >> >> Also attached is an updated test script. >> >> looks little bit strange, but it has sense +1 Pavel >> > -- > Fabien.
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v11. It is basically a simple rebase after Tom committed the "pgbench -M order" patch. It interfered because the compound command management also needs to delay part of the SQL command initialization. Some patch are luckier than others:-) Here is a v10: - does not talk about ASCII variable name constraint, as a patch has been submitted independently to lift this constraint. - rename gcset to cset (compound set, \; + \set), where gset is ; + \set, because "\gcset" looked really strange. - simplify the code a little bit. Also attached is an updated test script. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 03e1212..ea154bc 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -816,6 +816,51 @@ pgbench options dbname + + + \cset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \cset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \cset +SELECT 3 AS three \gset p_ + + + + + +\cset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae78c7b..b1488c0 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1225,6 +1228,104 @@ 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 **gset) +{ + PGresult *res; + int compound = 0; + + while ((res = PQgetResult(st->con)) != NULL) + { + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ +if (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, + varname); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + if (*gset[compound] != '\0') + free(varname); + } +} +break; /* OK */ + + default: +/*
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v3 with a more precise comment. Looks good to me. I have marked the patch status as "Ready for committer". Ok. Thanks. When/if committed, it might trigger a few rebase of other pending patches. I'll see about that then. -- 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 - allow to store select results into variables
> Hello, > >> I think you'd better to change the following comments because there's >> no psqlscan.l or psqlscanslash.l in pgbench source tree. >> >> + * underscore. Keep this in sync with the definition of >> variable_char in >> + * psqlscan.l and psqlscanslash.l. > > Here is a v3 with a more precise comment. Looks good to me. I have marked the patch status as "Ready for committer". Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
Hello, I think you'd better to change the following comments because there's no psqlscan.l or psqlscanslash.l in pgbench source tree. + * underscore. Keep this in sync with the definition of variable_char in + * psqlscan.l and psqlscanslash.l. Here is a v3 with a more precise comment. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..502d31b 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -825,6 +825,8 @@ pgbench options dbname Sets variable varname to a value calculated from expression. + The variable name must consist of letters (including non-Latin letters), + digits, and underscores. The expression may contain integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..1862fe4 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -58,9 +58,9 @@ extern void expr_yyset_column(int column_no, yyscan_t yyscanner); %option prefix="expr_yy" /* Character classes */ -alpha [a-zA-Z_] +alpha [a-zA-Z\200-\377_] digit [0-9] -alnum [a-zA-Z0-9_] +alnum [A-Za-z\200-\377_0-9] /* {space} + {nonspace} + {newline} should cover all characters */ space [ \t\r\f\v] nonspace [^ \t\r\f\v\n] diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..44ae29e 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -1020,19 +1020,38 @@ makeVariableNumeric(Variable *var) return true; } -/* check whether the name consists of alphabets, numerals and underscores. */ +/* + * Check whether a variable's name is allowed. + * + * We allow any non-ASCII character, as well as ASCII letters, digits, and + * underscore. + * + * Keep this in sync with the definitions of variable name characters in + * "src/fe_utils/psqlscan.l", "src/bin/psql/psqlscanslash.l" and + * "src/bin/pgbench/exprscan.l". + * + * Note: this static function is copied from "src/bin/psql/variables.c" + */ static bool -isLegalVariableName(const char *name) +valid_variable_name(const char *name) { - int i; + const unsigned char *ptr = (const unsigned char *) name; - for (i = 0; name[i] != '\0'; i++) + /* Mustn't be zero-length */ + if (*ptr == '\0') + return false; + + while (*ptr) { - if (!isalnum((unsigned char) name[i]) && name[i] != '_') + if (IS_HIGHBIT_SET(*ptr) || + strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" + "_0123456789", *ptr) != NULL) + ptr++; + else return false; } - return (i > 0);/* must be non-empty */ + return true; } /* @@ -1054,7 +1073,7 @@ lookupCreateVariable(CState *st, const char *context, char *name) * Check for the name only when declaring a new variable to avoid * overhead. */ - if (!isLegalVariableName(name)) + if (!valid_variable_name(name)) { fprintf(stderr, "%s: invalid variable name: \"%s\"\n", context, name); @@ -1148,7 +1167,7 @@ parseVariable(const char *sql, int *eaten) do { i++; - } while (isalnum((unsigned char) sql[i]) || sql[i] == '_'); + } while (IS_HIGHBIT_SET(sql[i]) || isalnum((unsigned char) sql[i]) || sql[i] == '_'); if (i == 1) return NULL; -- 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 - allow to store select results into variables
Fabien, > Hello Tatsuo-san, > >> Thank you for the patch. I tested a little bit and found that it does >> not allow value replacement against non ascii variables in given SQL >> statements . Is it intentional? > > No, this is a bug. > >> If not, I think you need to fix parseVariable() as well. > > Indeed. Here is v2. I think you'd better to change the following comments because there's no psqlscan.l or psqlscanslash.l in pgbench source tree. + * underscore. Keep this in sync with the definition of variable_char in + * psqlscan.l and psqlscanslash.l. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
It seems the new feature \gset doesn't work with tables having none ascii column names: Indeed. The same error is triggered with the \set syntax, which does not involve any query execution. I have added a sentence mentionning the restriction when variables are first discussed in the documentation, see attached patch. Here is a v10: - does not talk about ASCII variable name constraint, as a patch has been submitted independently to lift this constraint. - rename gcset to cset (compound set, \; + \set), where gset is ; + \set, because "\gcset" looked really strange. - simplify the code a little bit. Also attached is an updated test script. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..862e1d4 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -816,6 +816,51 @@ pgbench options dbname + + + \cset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \cset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \cset +SELECT 3 AS three \gset p_ + + + + + +\cset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..1e2402a 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1225,6 +1228,104 @@ 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 **gset) +{ + PGresult *res; + int compound = 0; + + while ((res = PQgetResult(st->con)) != NULL) + { + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ +if (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, + varname); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + if (*gset[compound] != '\0') +
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Tatsuo-san, Thank you for the patch. I tested a little bit and found that it does not allow value replacement against non ascii variables in given SQL statements . Is it intentional? No, this is a bug. If not, I think you need to fix parseVariable() as well. Indeed. Here is v2. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..502d31b 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -825,6 +825,8 @@ pgbench options dbname Sets variable varname to a value calculated from expression. + The variable name must consist of letters (including non-Latin letters), + digits, and underscores. The expression may contain integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..1862fe4 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -58,9 +58,9 @@ extern void expr_yyset_column(int column_no, yyscan_t yyscanner); %option prefix="expr_yy" /* Character classes */ -alpha [a-zA-Z_] +alpha [a-zA-Z\200-\377_] digit [0-9] -alnum [a-zA-Z0-9_] +alnum [A-Za-z\200-\377_0-9] /* {space} + {nonspace} + {newline} should cover all characters */ space [ \t\r\f\v] nonspace [^ \t\r\f\v\n] diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..f49e8c6 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -1020,19 +1020,35 @@ makeVariableNumeric(Variable *var) return true; } -/* check whether the name consists of alphabets, numerals and underscores. */ +/* + * Check whether a variable's name is allowed. + * + * We allow any non-ASCII character, as well as ASCII letters, digits, and + * underscore. Keep this in sync with the definition of variable_char in + * psqlscan.l and psqlscanslash.l. + * + * This static function is copied from "src/bin/psql/variables.c" + */ static bool -isLegalVariableName(const char *name) +valid_variable_name(const char *name) { - int i; + const unsigned char *ptr = (const unsigned char *) name; - for (i = 0; name[i] != '\0'; i++) + /* Mustn't be zero-length */ + if (*ptr == '\0') + return false; + + while (*ptr) { - if (!isalnum((unsigned char) name[i]) && name[i] != '_') + if (IS_HIGHBIT_SET(*ptr) || + strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" + "_0123456789", *ptr) != NULL) + ptr++; + else return false; } - return (i > 0);/* must be non-empty */ + return true; } /* @@ -1054,7 +1070,7 @@ lookupCreateVariable(CState *st, const char *context, char *name) * Check for the name only when declaring a new variable to avoid * overhead. */ - if (!isLegalVariableName(name)) + if (!valid_variable_name(name)) { fprintf(stderr, "%s: invalid variable name: \"%s\"\n", context, name); @@ -1148,7 +1164,7 @@ parseVariable(const char *sql, int *eaten) do { i++; - } while (isalnum((unsigned char) sql[i]) || sql[i] == '_'); + } while (IS_HIGHBIT_SET(sql[i]) || isalnum((unsigned char) sql[i]) || sql[i] == '_'); if (i == 1) return NULL; -- 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 - allow to store select results into variables
Fabien, >> As the variable infrastructures are pretty different between psql & >> pgbench (typed vs untyped values, sorted array vs linked list data >> structure, no hook vs 2 hooks, name spaces vs no such thing...), I >> have chosen the simplest option of just copying the name checking >> function and extending the lexer to authorize non-ascii letters, so >> that psql/pgbench would accept the same variable names with the same >> constraint about encodings. >> >> See patch attached & test script. > > Argh, I'm jet-lagged, wrong patch suffix... Here it is with the right > suffix. Thank you for the patch. I tested a little bit and found that it does not allow value replacement against non ascii variables in given SQL statements . Is it intentional? If not, I think you need to fix parseVariable() as well. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
As the variable infrastructures are pretty different between psql & pgbench (typed vs untyped values, sorted array vs linked list data structure, no hook vs 2 hooks, name spaces vs no such thing...), I have chosen the simplest option of just copying the name checking function and extending the lexer to authorize non-ascii letters, so that psql/pgbench would accept the same variable names with the same constraint about encodings. See patch attached & test script. Argh, I'm jet-lagged, wrong patch suffix... Here it is with the right suffix. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..502d31b 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -825,6 +825,8 @@ pgbench options dbname Sets variable varname to a value calculated from expression. + The variable name must consist of letters (including non-Latin letters), + digits, and underscores. The expression may contain integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..1862fe4 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -58,9 +58,9 @@ extern void expr_yyset_column(int column_no, yyscan_t yyscanner); %option prefix="expr_yy" /* Character classes */ -alpha [a-zA-Z_] +alpha [a-zA-Z\200-\377_] digit [0-9] -alnum [a-zA-Z0-9_] +alnum [A-Za-z\200-\377_0-9] /* {space} + {nonspace} + {newline} should cover all characters */ space [ \t\r\f\v] nonspace [^ \t\r\f\v\n] diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 78f1e6b..85f2edb 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -1020,19 +1020,35 @@ makeVariableNumeric(Variable *var) return true; } -/* check whether the name consists of alphabets, numerals and underscores. */ +/* + * Check whether a variable's name is allowed. + * + * We allow any non-ASCII character, as well as ASCII letters, digits, and + * underscore. Keep this in sync with the definition of variable_char in + * psqlscan.l and psqlscanslash.l. + * + * This static function is copied from "src/bin/psql/variables.c" + */ static bool -isLegalVariableName(const char *name) +valid_variable_name(const char *name) { - int i; + const unsigned char *ptr = (const unsigned char *) name; - for (i = 0; name[i] != '\0'; i++) + /* Mustn't be zero-length */ + if (*ptr == '\0') + return false; + + while (*ptr) { - if (!isalnum((unsigned char) name[i]) && name[i] != '_') + if (IS_HIGHBIT_SET(*ptr) || + strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" + "_0123456789", *ptr) != NULL) + ptr++; + else return false; } - return (i > 0);/* must be non-empty */ + return true; } /* @@ -1054,7 +1070,7 @@ lookupCreateVariable(CState *st, const char *context, char *name) * Check for the name only when declaring a new variable to avoid * overhead. */ - if (!isLegalVariableName(name)) + if (!valid_variable_name(name)) { fprintf(stderr, "%s: invalid variable name: \"%s\"\n", context, name); pgbench-utf8-vars.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
Re: [HACKERS] pgbench - allow to store select results into variables
As the variable infrastructures are pretty different between psql & pgbench (typed vs untyped values, sorted array vs linked list data structure, no hook vs 2 hooks, name spaces vs no such thing...), I have chosen the simplest option of just copying the name checking function and extending the lexer to authorize non-ascii letters, so that psql/pgbench would accept the same variable names with the same constraint about encodings. See patch attached & test script. Argh, I should be asleep:-( Wrong patch suffix, wrong from, multiple apology:-( Here it is again. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..502d31b 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -825,6 +825,8 @@ pgbench options dbname Sets variable varname to a value calculated from expression. + The variable name must consist of letters (including non-Latin letters), + digits, and underscores. The expression may contain integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..1862fe4 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -58,9 +58,9 @@ extern void expr_yyset_column(int column_no, yyscan_t yyscanner); %option prefix="expr_yy" /* Character classes */ -alpha [a-zA-Z_] +alpha [a-zA-Z\200-\377_] digit [0-9] -alnum [a-zA-Z0-9_] +alnum [A-Za-z\200-\377_0-9] /* {space} + {nonspace} + {newline} should cover all characters */ space [ \t\r\f\v] nonspace [^ \t\r\f\v\n] diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 78f1e6b..85f2edb 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -1020,19 +1020,35 @@ makeVariableNumeric(Variable *var) return true; } -/* check whether the name consists of alphabets, numerals and underscores. */ +/* + * Check whether a variable's name is allowed. + * + * We allow any non-ASCII character, as well as ASCII letters, digits, and + * underscore. Keep this in sync with the definition of variable_char in + * psqlscan.l and psqlscanslash.l. + * + * This static function is copied from "src/bin/psql/variables.c" + */ static bool -isLegalVariableName(const char *name) +valid_variable_name(const char *name) { - int i; + const unsigned char *ptr = (const unsigned char *) name; - for (i = 0; name[i] != '\0'; i++) + /* Mustn't be zero-length */ + if (*ptr == '\0') + return false; + + while (*ptr) { - if (!isalnum((unsigned char) name[i]) && name[i] != '_') + if (IS_HIGHBIT_SET(*ptr) || + strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" + "_0123456789", *ptr) != NULL) + ptr++; + else return false; } - return (i > 0);/* must be non-empty */ + return true; } /* @@ -1054,7 +1070,7 @@ lookupCreateVariable(CState *st, const char *context, char *name) * Check for the name only when declaring a new variable to avoid * overhead. */ - if (!isLegalVariableName(name)) + if (!valid_variable_name(name)) { fprintf(stderr, "%s: invalid variable name: \"%s\"\n", context, name); pgbench-utf8-vars.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
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Tatsuo-san, If I understand correctly, the patch is moved because of the unrelated issue that variables cannot be utf8 in pgbench, and it is a condition to consider this patch that existing pgbench variables (set with \set) can be utf8? I'm not sure if it is "unrelated" because the new feature relies on existing pgbench variable infrastructure. Sure. I meant that the constraint on variable names exists before the patch and the patch is not related to variable names, but the patch is about variables, obviously. As "psql" variables can be utf8 and that the same scanner is used, but the variables values are not stritcly the same (they are typed in pgbench), I'm wondering whether the effort should be do share more code/abstraction between psql & pgbench or just adjust/replicate the needed small functions/code excerpts. As the variable infrastructures are pretty different between psql & pgbench (typed vs untyped values, sorted array vs linked list data structure, no hook vs 2 hooks, name spaces vs no such thing...), I have chosen the simplest option of just copying the name checking function and extending the lexer to authorize non-ascii letters, so that psql/pgbench would accept the same variable names with the same constraint about encodings. See patch attached & test script. -- Fabien. pgbench-utf8-vars-1.sql Description: application/sql pgbench-utf8-vars.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
Re: [HACKERS] pgbench - allow to store select results into variables
If I understand correctly, the patch is moved because of the unrelated issue that variables cannot be utf8 in pgbench, and it is a condition to consider this patch that existing pgbench variables (set with \set) can be utf8? I'm not sure if it is "unrelated" because the new feature relies on existing pgbench variable infrastructure. Sure. I meant that the constraint on variable names exists before the patch and the patch is not related to variable names, but the patch is about variables, obviously. As "psql" variables can be utf8 and that the same scanner is used, but the variables values are not stritcly the same (they are typed in pgbench), I'm wondering whether the effort should be do share more code/abstraction between psql & pgbench or just adjust/replicate the needed small functions/code excerpts. -- 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 - allow to store select results into variables
> If I understand correctly, the patch is moved because of the unrelated > issue that variables cannot be utf8 in pgbench, and it is a condition > to consider this patch that existing pgbench variables (set with \set) > can be utf8? I'm not sure if it is "unrelated" because the new feature relies on existing pgbench variable infrastructure. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
Hello Tatsuo, Ok, I will move the patch to the next cf. Done. If I understand correctly, the patch is moved because of the unrelated issue that variables cannot be utf8 in pgbench, and it is a condition to consider this patch that existing pgbench variables (set with \set) can be utf8? -- 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 - allow to store select results into variables
>> Having said all that, I think we're at the point in the commitfest >> where if there's any design question at all about a patch, it should >> get booted to the next cycle. We are going to have more than enough >> to do to stabilize what's already committed, we don't need to be >> adding more uncertainty. > > Ok, I will move the patch to the next cf. Done. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
> Well, personally, as an all-ASCII guy I'm not too fussed about that, > but I can see that other people might be annoyed. > > The main problem in dealing with it seems to be whether you're willing > to support pgbench running in non-backend-safe encodings (eg SJIS). > If we rejected that case then it'd be a relatively simple change to allow > pgbench to treat any high-bit-set byte as a valid variable name character. > (I think anyway, haven't checked the code.) > > Although ... actually, psql allows any high-bit-set byte in variable > names (cf valid_variable_name()) without concern about encoding. > That means it's formally incorrect in SJIS, but it's been like that > for an awful lot of years and nobody's complained. Maybe it'd be fine > for pgbench to act the same. That's my thought too. > Having said all that, I think we're at the point in the commitfest > where if there's any design question at all about a patch, it should > get booted to the next cycle. We are going to have more than enough > to do to stabilize what's already committed, we don't need to be > adding more uncertainty. Ok, I will move the patch to the next cf. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
On 2017-04-05 20:24:19 -0400, Tom Lane wrote: > Having said all that, I think we're at the point in the commitfest > where if there's any design question at all about a patch, it should > get booted to the next cycle. We are going to have more than enough > to do to stabilize what's already committed, we don't need to be > adding more uncertainty. +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 - allow to store select results into variables
Tatsuo Ishiiwrites: > I still wonder whether I should commit this or not because this patch > does not allow none ascii column names. Well, personally, as an all-ASCII guy I'm not too fussed about that, but I can see that other people might be annoyed. The main problem in dealing with it seems to be whether you're willing to support pgbench running in non-backend-safe encodings (eg SJIS). If we rejected that case then it'd be a relatively simple change to allow pgbench to treat any high-bit-set byte as a valid variable name character. (I think anyway, haven't checked the code.) Although ... actually, psql allows any high-bit-set byte in variable names (cf valid_variable_name()) without concern about encoding. That means it's formally incorrect in SJIS, but it's been like that for an awful lot of years and nobody's complained. Maybe it'd be fine for pgbench to act the same. Having said all that, I think we're at the point in the commitfest where if there's any design question at all about a patch, it should get booted to the next cycle. We are going to have more than enough to do to stabilize what's already committed, we don't need to be adding more uncertainty. 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 - allow to store select results into variables
Tom and others, I still wonder whether I should commit this or not because this patch does not allow none ascii column names. We know pgbench variable name has been restricted since the functionality was born. When users explicitly define a pgbench variable using \set, it is not a too strong limitation, because it's in a "pgbench world" anyway and nothing is related to PostgreSQL core specs. However, \gset is not happy with perfectly valid column names in PostgreSQL core, which looks too inconsistent and confusing for users. So the choices are: 1) commit the patch now with documenting the limitation. (the patch looks good to me except the issue above) 2) move it to next cf hoping that someone starts the implementation to eliminate the limitation of none ascii variable names. Comments? Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp > Hello Tatsuo-san, > >> It seems the new feature \gset doesn't work with tables having none >> ascii column names: > > Indeed. The same error is triggered with the \set syntax, which does > not involve any query execution. > > I have added a sentence mentionning the restriction when variables are > first discussed in the documentation, see attached patch. > > -- > 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 - allow to store select results into variables
Hello Tatsuo-san, It seems the new feature \gset doesn't work with tables having none ascii column names: Indeed. The same error is triggered with the \set syntax, which does not involve any query execution. I have added a sentence mentionning the restriction when variables are first discussed in the documentation, see attached patch. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..c40f73e 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -773,6 +773,7 @@ pgbench options dbname There is a simple variable-substitution facility for script files. Variables can be set by the command-line -D option, explained above, or by the meta commands explained below. + Variable names are restricted to ASCII strings. In addition to any variables preset by -D command-line options, there are a few variables that are preset automatically, listed in . A value specified for these @@ -816,6 +817,51 @@ pgbench options dbname + + + \gcset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \gcset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \gcset +SELECT 3 AS three \gset p_ + + + + + +\gcset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 78f1e6b..ac9289b 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1225,6 +1228,105 @@ 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 **gset) +{ + 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 (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, +
Re: [HACKERS] pgbench - allow to store select results into variables
>> Please find attached a v8 which hopefully fixes these two issues. > Looks good to me, marking as ready for committer. I have looked into this a little bit. It seems the new feature \gset doesn't work with tables having none ascii column names: $ src/bin/pgbench/pgbench -t 1 -f /tmp/f test starting vacuum...end. gset: invalid variable name: "数字" client 0 file 0 command 0 compound 0: error storing into var 数字 transaction type: /tmp/f scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 number of transactions per client: 1 number of transactions actually processed: 0/1 This is because pgbench variable names are limited to ascii ranges. IMO the limitation is unnecessary and should be removed. (I know that the limitation was brought in by someone long time ago and the patch author is not responsible for that). Anyway, now that the feature reveals the undocumented limitation, we should document the limitation of \gset at least. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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 - allow to store select results into variables
On Fri, Mar 24, 2017 at 8:59 PM, Fabien COELHOwrote: > > Hello Rafia, > >> if (my_command->argc > 2) >> + syntax_error(source, lineno, my_command->line, my_command->argv[0], >> + "at most on argument expected", NULL, -1); >> >> I suppose you mean 'one' argument here. > > > Indeed. > >> Apart from that indentation is not correct as per pgindent, please check. > > > I guess that you are refering to switch/case indentation which my emacs does > not do as expected. > > Please find attached a v8 which hopefully fixes these two issues. Looks good to me, marking as ready for committer. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -- 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 - allow to store select results into variables
Hello Rafia, if (my_command->argc > 2) + syntax_error(source, lineno, my_command->line, my_command->argv[0], + "at most on argument expected", NULL, -1); I suppose you mean 'one' argument here. Indeed. Apart from that indentation is not correct as per pgindent, please check. I guess that you are refering to switch/case indentation which my emacs does not do as expected. Please find attached a v8 which hopefully fixes these two issues. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 1eee8dc..1108fcb 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -816,6 +816,51 @@ pgbench options dbname + + + \gcset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \gcset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \gcset +SELECT 3 AS three \gset p_ + + + + + +\gcset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 78f1e6b..ac9289b 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1225,6 +1228,105 @@ 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 **gset) +{ + 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 (gset[compound] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); + st->ecnt++; + return false; +} +break; /* OK */ + + case PGRES_TUPLES_OK: +if (gset[compound] != NULL) +{ + /* store result into variables */ + int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + + if (ntuples != 1) + { + fprintf(stderr, +"client %d file %d command %d compound %d: " +"expecting one row, got %d\n", +st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + for (f = 0; f < nfields ; f++) + { + char *varname = PQfname(res, f); + if (*gset[compound] != '\0') + varname = psprintf("%s%s", gset[compound], varname); + + /* store result as a string */ + if (!putVariable(st, "gset", varname, + PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "error storing into var %s\n", + st->id, st->use_file, st->command, compound, + varname); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + if (*gset[compound] != '\0') + free(varname); + } +} +break; /* OK */ + + default: +/* everything else is unexpected, so probably an error */ +fprintf(stderr, + "client %d file %d aborted in command
Re: [HACKERS] pgbench - allow to store select results into variables
On Thu, Mar 16, 2017 at 12:45 AM, Fabien COELHOwrote: > > Hello Rafia, > >> I was reviewing v7 of this patch, to start with I found following white >> space errors when applying with git apply, >> /home/edb/Desktop/patches/others/pgbench-into-7.patch:66: trailing >> whitespace. > > > Yep. > > I do not know why "git apply" sometimes complains. All is fine for me both > with "git apply" and "patch". > > Last time it was because my mailer uses text/x-diff for the mime type, as > define by the system in "/etc/mime.types", which some mailer then interpret > as a license to change eol-style when saving, resulting in this kind of > behavior. Could you tell your mailer just to save the file as is? > >> Apart from that, on executing SELECT 1 AS a \gset \set i debug(:a) SELECT >> 2 >> AS a \gcset SELECT 3; given in your provided script gset-1.sql. it is >> giving error Invalid command \gcset. > > > Are you sure that you are using the compiled pgbench, not a previously > installed one? > > bin/pgbench> pgbench -t 1 -f SQL/gset-1.sql > SQL/gset-1.sql:1: invalid command in command "gset" > \gset > > bin/pgbench> ./pgbench -t 1 -f SQL/gset-1.sql > starting vacuum...end. > debug(script=0,command=2): int 1 > debug(script=0,command=4): int 2 > ... > >> Not sure what is the intention of this script anyway? > > > The intention is to test that gset & gcset work as expected in various > settings, especially with combined queries (\;) the right result must be > extracted in the sequence. > >> Also, instead of so many different files for error why don't you combine >> it into one. > > > Because a pgbench scripts stops on the first error, and I wanted to test > what happens with several kind of errors. > if (my_command->argc > 2) + syntax_error(source, lineno, my_command->line, my_command->argv[0], + "at most on argument expected", NULL, -1); I suppose you mean 'one' argument here. Apart from that indentation is not correct as per pgindent, please check. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ -- 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 - allow to store select results into variables
Hello Rafia, I was reviewing v7 of this patch, to start with I found following white space errors when applying with git apply, /home/edb/Desktop/patches/others/pgbench-into-7.patch:66: trailing whitespace. Yep. I do not know why "git apply" sometimes complains. All is fine for me both with "git apply" and "patch". Last time it was because my mailer uses text/x-diff for the mime type, as define by the system in "/etc/mime.types", which some mailer then interpret as a license to change eol-style when saving, resulting in this kind of behavior. Could you tell your mailer just to save the file as is? Apart from that, on executing SELECT 1 AS a \gset \set i debug(:a) SELECT 2 AS a \gcset SELECT 3; given in your provided script gset-1.sql. it is giving error Invalid command \gcset. Are you sure that you are using the compiled pgbench, not a previously installed one? bin/pgbench> pgbench -t 1 -f SQL/gset-1.sql SQL/gset-1.sql:1: invalid command in command "gset" \gset bin/pgbench> ./pgbench -t 1 -f SQL/gset-1.sql starting vacuum...end. debug(script=0,command=2): int 1 debug(script=0,command=4): int 2 ... Not sure what is the intention of this script anyway? The intention is to test that gset & gcset work as expected in various settings, especially with combined queries (\;) the right result must be extracted in the sequence. Also, instead of so many different files for error why don't you combine it into one. Because a pgbench scripts stops on the first error, and I wanted to test what happens with several kind of errors. -- 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 - allow to store select results into variables
On Tue, Jan 31, 2017 at 11:54 AM, Fabien COELHOwrote: > > Bonjour Michaël, > > Attached are the patch, a test script for the feature, and various test >>> scripts to trigger error cases. >>> >> >> I have moved this patch to next CF >> > > Ok. > > as the last status is a new patch set with no further reviews. >> > > Indeed. > > I did not check if the comments have been applied though, this is a bit >> too much for me now... >> > > Sure. I was reviewing v7 of this patch, to start with I found following white space errors when applying with git apply, /home/edb/Desktop/patches/others/pgbench-into-7.patch:66: trailing whitespace. char *line; /* first line for short display */ /home/edb/Desktop/patches/others/pgbench-into-7.patch:67: trailing whitespace. char *lines; /* full multi-line text of command */ /home/edb/Desktop/patches/others/pgbench-into-7.patch:72: trailing whitespace. int compound; /* last compound command (number of \;) */ /home/edb/Desktop/patches/others/pgbench-into-7.patch:73: trailing whitespace. char **gset; /* per-compound command prefix */ /home/edb/Desktop/patches/others/pgbench-into-7.patch:81: trailing whitespace. /* read all responses from backend */ error: patch failed: doc/src/sgml/ref/pgbench.sgml:815 error: doc/src/sgml/ref/pgbench.sgml: patch does not apply error: patch failed: src/bin/pgbench/pgbench.c:375 error: src/bin/pgbench/pgbench.c: patch does not apply error: patch failed: src/bin/pgbench/pgbench.h:11 error: src/bin/pgbench/pgbench.h: patch does not apply error: patch failed: src/fe_utils/psqlscan.l:678 error: src/fe_utils/psqlscan.l: patch does not apply error: patch failed: src/include/fe_utils/psqlscan_int.h:112 error: src/include/fe_utils/psqlscan_int.h: patch does not apply Apart from that, on executing SELECT 1 AS a \gset \set i debug(:a) SELECT 2 AS a \gcset SELECT 3; given in your provided script gset-1.sql. it is giving error Invalid command \gcset. Not sure what is the intention of this script anyway? Also, instead of so many different files for error why don't you combine it into one. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
Re: [HACKERS] pgbench - allow to store select results into variables
Bonjour Michaël, Attached are the patch, a test script for the feature, and various test scripts to trigger error cases. I have moved this patch to next CF Ok. as the last status is a new patch set with no further reviews. Indeed. I did not check if the comments have been applied though, this is a bit too much for me now... Sure. -- 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 - allow to store select results into variables
On Sat, Jan 7, 2017 at 6:25 PM, Fabien COELHOwrote: > I think that I have done what you required. > > I have documented the fact that now the feature does not work if compound > commands contain empty queries, which is a very minor drawback for a pgbench > script anyway. > > Attached are the patch, a test script for the feature, and various test > scripts to trigger error cases. I have moved this patch to next CF as the last status is a new patch set with no further reviews. I did not check if the comments have been applied though, this is a bit too much for me now.. -- Michael -- 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 - allow to store select results into variables
Please pardon the redondance: this is a slightly edited repost from another thread where motivation for this patch was discussed, so that it appear in the relevant thread. Tom> [...] there was immediately objection as to whether his idea of TPC-B Tom> compliance was actually right. From my point of view TPC-* are simply objective examples of typical benchmark requirements to show which features are needed in a tool for doing this activity. Once features are available, I think that pgbench should also be a show-case for their usage. Currently a few functions (for implementing the bench as specified) and actually extracting results into variables (for suspicious auditors and bench relevance, see below) are missing. Tom> I remember complaining that he had a totally artificial idea of what Tom> "fetching a data value" requires. Yep. I think that the key misunderstanding is that you are honest and assume that other people are honest too. This is naïve: There is a long history of vendors creatively "cheating" to get better than deserve benchmark results. Benchmark specifications try to prevent such behaviors by laying careful requirements and procedures. In this instance, you "know" that when pg has returned the result of the query the data is actually on the client side, so you considered it is fetched. That is fine for you, but from a benchmarking perspective with external auditors your belief/knowledge is not good enough. For instance, the vendor could implement a new version of the protocol where the data are only transfered on demand, and the result just tells that the data is indeed somewhere on the server (eg on "SELECT abalance" it could just check that the key exists, no need to actually fetch the data from the table, so no need to read the table, the index is enough...). That would be pretty stupid for real application performance, but the benchmark would get better tps by doing so. Without even intentionnaly cheating, this could be part of a useful "streaming mode" protocol option which make sense for very large results but would be activated for a small result. Another point is that decoding the message may be a little expensive, so that by not actually extracting the data into the client but just keeping it in the connection/OS one gets better performance. Thus, TPC-B 2.0.0 benchmark specification says: "1.3.2 Each transaction shall return to the driver the Account_Balance resulting from successful commit of the transaction. Comment: It is the intent of this clause that the account balance in the database be returned to the driver, i.e., that the application retrieve the account balance." For me the correct interpretation of "the APPLICATION retrieve the account balance" is that the client application code, pgbench in this context, did indeed get the value from the vendor code, here "libpq" which is handling the connection. Having the value discarded from libpq by calling PQclear instead of PQntuples/PQgetvalue/... skips a key part of the client code that no real application would skip. This looks strange and is not representative of real client code: as a potential auditor, because of this performance impact doubt and lack of relevance, I would not check the corresponding item in the audit check list: "11.3.1.2 Verify that transaction inputs and outputs satisfy Clause 1.3." So the benchmark implementation would not be validated. Another trivial reason to be able to actually retrieve data is that for benchmarking purpose it is very easy to want to test a scenario where you do different things based on data received, which imply that the data can be manipulated somehow on the benchmarking client side, which is currently not possible. -- 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 - allow to store select results into variables
Hello Tom, Please look at changing \into to be a SQL-command-ending backslash command as we previously discussed. Done. There are two variants: \gset & \gcset for compound (end SQL query, set variables, but do not end command, so that several settings are allowed in a compound command, a key feature for performance testing). Personnally, I find the end-of-query semicolon-replacing syntax ugly. However I'm more interested in feature than in elegance on this one, so I'll put up with it. I think you will find that the implementation is a great deal simpler that way and doesn't require weird hackery on the shared lexer. I have removed the "hackery", only counting embedded semicolons remains to keep track of compound queries. Note that the (somehow buggy and indeed not too clean) hackery was not related to the into syntax, but to detecting empty queries which are silently skipped by the server. If you won't do that, [...] I think that I have done what you required. I have documented the fact that now the feature does not work if compound commands contain empty queries, which is a very minor drawback for a pgbench script anyway. Attached are the patch, a test script for the feature, and various test scripts to trigger error cases. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 3fb29f8..9a0fb12 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -815,6 +815,51 @@ pgbench options dbname + + + \gcset [prefix] or + \gset [prefix] + + + + + These commands may be used to end SQL queries, replacing a semicolon. + \gcset replaces an embedded semicolon (\;) within + a compound SQL command, and \gset replaces a final + (;) semicolon which ends the SQL command. + + + + When these commands are used, the preceding SQL query is expected to + return one row, the columns of which are stored into variables named after + column names, and prefixed with prefix if provided. + + + + The following example puts the final account balance from the first query + into variable abalance, and fills variables + one, two and p_three with + integers from a compound query. + +UPDATE pgbench_accounts + SET abalance = abalance + :delta + WHERE aid = :aid + RETURNING abalance \gset +-- compound of two queries +SELECT 1 AS one, 2 AS two \gcset +SELECT 3 AS three \gset p_ + + + + + +\gcset and \gset commands do not work when +empty SQL queries appear within a compound SQL command. + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index f6cb5d4..d57eb31 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -375,11 +375,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 **gset; /* per-compound command prefix */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1225,6 +1228,105 @@ 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 **gset) +{ + 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 (gset[compound] != NULL) + { +fprintf(stderr, + "client %d file %d command %d compound %d: " + "\\gset expects a row\n", + st->id, st->use_file, st->command, compound); +st->ecnt++; +return false; + } + break; /* OK */ + + case PGRES_TUPLES_OK: + if (gset[compound] != NULL) + { +/* store result into variables */ +int ntuples = PQntuples(res), + nfields = PQnfields(res), + f; + +if (ntuples != 1) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: " + "expecting one row, got %d\n", + st->id, st->use_file, st->command, compound, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; +} + +for (f = 0; f < nfields ; f++) +{ + char *varname = PQfname(res, f); + if (*gset[compound]
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Tom, Please look at changing \into to be a SQL-command-ending backslash command as we previously discussed. Hmmm. I do want storing results & compound command ending to be orthogonal. In order to keep this feature, I think that I can move the "into/ginto/gset/..." at the end of the command. For the compound command list to necessarily end, I can probably do some reassembly as a post phase on Commands in pgbench so that the impact on the lexer is much reduced, in particular without undue "hackery" as you put it. -- 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 - allow to store select results into variables
Fabien COELHOwrites: > Indeed. Here is the rebased version, which still get through my various > tests. I looked through this again, and I still think that the syntactic design of the new command is seriously misguided, leading to an ugly and unmaintainable implementation that may well block further innovation in pgbench. I will not commit it in this form. Possibly you can find some other committer whom you can convince this is a good design --- but since the patch has gone untouched for two full commitfest cycles, I rather imagine that whoever else has looked at it has likewise decided they didn't want to be responsible for it. Please look at changing \into to be a SQL-command-ending backslash command as we previously discussed. I think you will find that the implementation is a great deal simpler that way and doesn't require weird hackery on the shared lexer. (BTW, said hackery is not just weird but broken. You can't simply remove comments. Consider something like "SELECT foo/*as*/bar". This code reduces that to "SELECT foobar" which is wrong.) If you won't do that, and you can't find another committer who will accept responsibility for this patch before the end of the current commitfest, I think we should mark it Rejected. 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 - allow to store select results into variables
On Mon, Oct 3, 2016 at 12:43 AM, Michael Paquierwrote: > On Tue, Sep 27, 2016 at 10:41 AM, Amit Langote > wrote: > > On 2016/09/26 20:27, Fabien COELHO wrote: > >> > >> Hello Amit, > >> > I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as > I > have no further comments at the moment. > >>> > >>> Wait... Heikki's latest commit now requires this patch to be rebased. > >> > >> Indeed. Here is the rebased version, which still get through my various > >> tests. > > > > Thanks, Fabien. It seems to work here too. > > Moved to next CF with same status, ready for committer. > Moved to next CF with same status (ready for committer). Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] pgbench - allow to store select results into variables
On Tue, Sep 27, 2016 at 10:41 AM, Amit Langotewrote: > On 2016/09/26 20:27, Fabien COELHO wrote: >> >> Hello Amit, >> I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I have no further comments at the moment. >>> >>> Wait... Heikki's latest commit now requires this patch to be rebased. >> >> Indeed. Here is the rebased version, which still get through my various >> tests. > > Thanks, Fabien. It seems to work here too. Moved to next CF with same status, ready for committer. -- Michael -- 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 - allow to store select results into variables
On 2016/09/26 20:27, Fabien COELHO wrote: > > Hello Amit, > >>> I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I >>> have no further comments at the moment. >> >> Wait... Heikki's latest commit now requires this patch to be rebased. > > Indeed. Here is the rebased version, which still get through my various > tests. Thanks, Fabien. It seems to work here too. Regards, Amit -- 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 - allow to store select results into variables
Hello Amit, I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I have no further comments at the moment. Wait... Heikki's latest commit now requires this patch to be rebased. Indeed. Here is the rebased version, which still get through my various tests. -- 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 options dbname + + + \into var1 [var2 ...] + + + + + 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. + + + + Example: + +SELECT abalance \into abalance + FROM pgbench_accounts WHERE aid=5432; + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 1fb4ae4..9c13a18 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -373,11 +373,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; @@ -1221,6 +1224,110 @@ 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 file %d command %d compound %d: " + "\\into expects a row\n", + st->id, st->use_file, st->command, 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 file %d command %d compound %d: " + "expecting one row, got %d\n", + st->id, st->use_file, st->command, 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 file %d command %d compound %d: " +"error storing into var %s\n", +st->id, st->use_file, st->command, compound, +intovars[compound][f]); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + f++; +} + +if (intovars[compound][f] != NULL) +{ + fprintf(stderr, + "client %d file %d command %d compound %d: missing results" + " to fill into variable %s\n", + st->id, st->use_file, st->command, compound, + intovars[compound][f]); + st->ecnt++; + return false; +} + } + break; /* OK */ + + default: + /* everything else is unexpected, so probably an error */ + fprintf(stderr, "client %d file %d aborted in command %d compound %d: %s", + st->id, st->use_file, st->command, compound, + PQerrorMessage(st->con)); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + PQclear(res); + } + + if (compound == -1) + { + fprintf(stderr, "client %d command %d: no results\n", st->id, st->command); + 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) @@ -1953,7 +2060,6 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs) static void doCustom(TState *thread, CState *st, StatsData *agg) { -
Re: [HACKERS] pgbench - allow to store select results into variables
On 2016/09/26 16:12, Amit Langote wrote: > I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I > have no further comments at the moment. Wait... Heikki's latest commit now requires this patch to be rebased. commit 12788ae49e1933f463bc59a6efe46c4a01701b76 Author: Heikki LinnakangasDate: Mon Sep 26 10:56:02 2016 +0300 Refactor script execution state machine in pgbench. So, will change the status to "Waiting on Author". Thanks, Amit -- 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 - allow to store select results into variables
Hi Fabien, I am marking the pgbench-into-5.patch [1] as "Ready for Committer" as I have no further comments at the moment. Thanks, Amit [1] https://www.postgresql.org/message-id/alpine.DEB.2.20.1609130730380.10870%40lancre -- 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 - allow to store select results into variables
Hello Amit, [...] Then you would get the -r cut at the end of the compound command. Thus the current version gives full control of what will appear in the summary. If I change "\into xxx\n" to mean "also cut here", then there is less control on when the cut occurs when into is used. So it means that position of \into affects where a compound command gets cut for -r display. I was just wondering if that was unintentional. Yes, but it happens to work reasonnably:-) The other thing I have considered is whether to implemented a "\gset" syntax, as suggested by Pavel and Tom. Bar the aesthetic, the main issue I have with it is that it does not work with compound commands, and what I want is to get the values out of compound commands... because of my focus on latency... so basically "\gset" does not do the job I want... Now I recognize that other people would like it, so probably I'll do it anyway in another patch. So, psql's \gset does not work as desired for compound commands (viz. it is only able to save the result of the last sub-command). Yes. You need to use \gset with each sub-command separately if no result should be discarded. Because of undesirable latency characteristics of sending multiple commands, you want to be able to modify compound command handling such that every sub-command's result could be saved. Exactly. In that regard, it's good that pgbench does not use PQexec() which only returns the result of the last sub-command if a compound command was issued through it. Indeed! pgbench's doCustom() currently discards all results by issuing discard_response(). You propose to change things such that results are intercepted in a new function read_response(), values assigned to intovars corresponding to each sub-command, and then discarded. The intovars arrays are allocated within each sub-command's Command struct when parsing the compound command based on where \into is located wrt to the sub-command. Yep. So, most of the code in the patch is about handling compound statements to be be able to save results of all sub-commands, not just the last. Yep. Previously pgbench did not need to handle compounds commands which where just seen as one large string. Note that the added machinery is also a first step to allow the handling of prepared statements on compounds command, which I think is a desirable feature for benchmarks. Do you think it would be OK to suffer the bad latency of multiple round trips and implement a version of \into (or \gset or \ginto) for pgbench scripts that behaves exactly like psql's \gset as a first step? I do not see gset as a reasonnable "first step" because: (0) "\into" already works while "\gset" in pgbench will need some time that I do not have at the moment (1) it is not what I want/need to do a clean bench (2) the feature is not orthogonal to compounds statements -- which is what I want -- (3) I do not like the "implicit" naming thing -- but this is really just a matter of taste. I simply recognize that Peter & Tom have a point: whatever I think of gset it is there in "psql" so it makes some sense to have it as well in "pgbench", so I agree to put that on my pgbench todo list. But you say you will do it as another patch. Yep. I suggested another patch because it is a different feature and previous submissions where I mixed features, even closely related ones, all resulted in me having to separate the features in distinct patches. By the way, I tend to agree with your point about \gset syntax being suitable (only) in a interactive context such as psql; it's not as readable as \into x y ... when used in a script. Yep, but as people already know it, it makes sense to provide it as well at some point. -- 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 - allow to store select results into variables
Hi Fabien, On 2016/09/13 17:41, Fabien COELHO wrote: > > Hello Amit, > >> [...] >> There still seems to be a change in behavior of the -r option due to the >> patch. Consider the following example: >> >> select a from a where a = 1 \; >> select a+1 from a where a = 1; >> ... >> - statement latencies in milliseconds: >> 2.889 select a from a where a = 1 ; > > vs > >> select a from a where a = 1 \; \into a >> select a+1 from a where a = 1; \into b >> ... >> 2.093 select a from a where a = 1 ; select a+1 from a where a = 1; > > Yep. > > Note that there is a small logical conumdrum in this argument: As the > script is not the same, especially as into was not possible before, > strictly speaking there is no behavior "change". Sure, scripts are not the same but it seemed like showing the whole compound query whereas previously only part of it was shown may be an implementation artifact of \into. > This said, what you suggest can be done. > > After giving it some thought, I suggest that it is not needed nor > desirable. If you want to achieve the initial effect, you just have to put > the "into a" on the next line: > > select a from a where a = 1 \; > \into a > select a+1 from a where a = 1; \into b > > Then you would get the -r cut at the end of the compound command. Thus the > current version gives full control of what will appear in the summary. If > I change "\into xxx\n" to mean "also cut here", then there is less control > on when the cut occurs when into is used. So it means that position of \into affects where a compound command gets cut for -r display. I was just wondering if that was unintentional. >> One more thing I observed which I am not sure if it's a fault of this >> patch is illustrated below: >> >> $ cat /tmp/into.sql >> \; >> select a from a where a = 1 \; >> select a+1 from a where a = 1; >> >> $ pgbench -r -n -t 1 -f /tmp/into.sql postgres >> >> - statement latencies in milliseconds: >> 2.349 ; >> >> Note that the compound select statement is nowhere to be seen in the >> latencies output. The output remains the same even if I use the \into's. >> What seems to be going on is that the empty statement on the first line >> (\;) is the only part kept of the compound statement spanning lines 1-3. > > Yes. > > This is really the (debatable) current behavior, and is not affected by > the patch. The "-r" summary takes the first line of the command, whatever > it is. In your example the first line is "\;", so you get what you asked > for, even if it looks rather strange, obviously. Yep, perhaps it's strange to write a script like that anyway, :) +boolsql_command_in_progress = false; >> [...] >> I understood that it refers to what you explain here. But to me it >> sounded like the name is referring to the progress of *execution* of a SQL >> command whereas the code in question is simply expecting to finish >> *parsing* the SQL command using the next lines. > > Ok. I changed it "sql_command_lexing_in_progress". > >>> 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 >> >> The patch seems fine without these, although please consider the concern I >> raised with regard to the -r option above. > > I have considered it. As the legitimate behavior you suggested can be > achieved just by putting the into on the next line, ISTM that the current > proposition gives more control than doing a mandatory cut when into is used. > > Attached is a new version with the boolean renaming. Thanks. > The other thing I have considered is whether to implemented a "\gset" > syntax, as suggested by Pavel and Tom. Bar the aesthetic, the main issue I > have with it is that it does not work with compound commands, and what I > want is to get the values out of compound commands... because of my focus > on latency... so basically "\gset" does not do the job I want... Now I > recognize that other people would like it, so probably I'll do it anyway > in another patch. So, psql's \gset does not work as desired for compound commands (viz. it is only able to save the result of the last sub-command). You need to use \gset with each sub-command separately if no result should be discarded. Because of undesirable latency characteristics of sending multiple commands, you want to be able to modify compound command handling such that every sub-command's result could be saved. In that regard, it's good that pgbench does not use PQexec() which only returns the result of the last sub-command if a compound command was issued through it. pgbench's doCustom() currently discards all results by issuing discard_response(). You propose to change things such that results are intercepted in a new function read_response(), values assigned to intovars corresponding to each sub-command, and then discarded. The intovars
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Amit, [...] There still seems to be a change in behavior of the -r option due to the patch. Consider the following example: select a from a where a = 1 \; select a+1 from a where a = 1; ... - statement latencies in milliseconds: 2.889 select a from a where a = 1 ; vs select a from a where a = 1 \; \into a select a+1 from a where a = 1; \into b ... 2.093 select a from a where a = 1 ; select a+1 from a where a = 1; Yep. Note that there is a small logical conumdrum in this argument: As the script is not the same, especially as into was not possible before, strictly speaking there is no behavior "change". This said, what you suggest can be done. After giving it some thought, I suggest that it is not needed nor desirable. If you want to achieve the initial effect, you just have to put the "into a" on the next line: select a from a where a = 1 \; \into a select a+1 from a where a = 1; \into b Then you would get the -r cut at the end of the compound command. Thus the current version gives full control of what will appear in the summary. If I change "\into xxx\n" to mean "also cut here", then there is less control on when the cut occurs when into is used. One more thing I observed which I am not sure if it's a fault of this patch is illustrated below: $ cat /tmp/into.sql \; select a from a where a = 1 \; select a+1 from a where a = 1; $ pgbench -r -n -t 1 -f /tmp/into.sql postgres - statement latencies in milliseconds: 2.349 ; Note that the compound select statement is nowhere to be seen in the latencies output. The output remains the same even if I use the \into's. What seems to be going on is that the empty statement on the first line (\;) is the only part kept of the compound statement spanning lines 1-3. Yes. This is really the (debatable) current behavior, and is not affected by the patch. The "-r" summary takes the first line of the command, whatever it is. In your example the first line is "\;", so you get what you asked for, even if it looks rather strange, obviously. +boolsql_command_in_progress = false; [...] I understood that it refers to what you explain here. But to me it sounded like the name is referring to the progress of *execution* of a SQL command whereas the code in question is simply expecting to finish *parsing* the SQL command using the next lines. Ok. I changed it "sql_command_lexing_in_progress". 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 The patch seems fine without these, although please consider the concern I raised with regard to the -r option above. I have considered it. As the legitimate behavior you suggested can be achieved just by putting the into on the next line, ISTM that the current proposition gives more control than doing a mandatory cut when into is used. Attached is a new version with the boolean renaming. The other thing I have considered is whether to implemented a "\gset" syntax, as suggested by Pavel and Tom. Bar the aesthetic, the main issue I have with it is that it does not work with compound commands, and what I want is to get the values out of compound commands... because of my focus on latency... so basically "\gset" does not do the job I want... Now I recognize that other people would like it, so probably I'll do it anyway in another patch. -- 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 options dbname + + + \into var1 [var2 ...] + + + + + 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. + + + + Example: + +SELECT abalance \into abalance + FROM pgbench_accounts WHERE aid=5432; + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 56c37d5..8817ac5 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
Re: [HACKERS] pgbench - allow to store select results into variables
Hi Fabien, On 2016/09/07 23:01, Fabien COELHO wrote: >> 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. Seems to be fixed. >> Comments on the patch follow: >> >> + >> + >> + Stores the first fields of the resulting row from the current or >> preceding >> + SELECT command into these variables. >> >> Any command returning rows ought to work, no? > > Yes. I put "SQL command" instead. Check. >> -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. There still seems to be a change in behavior of the -r option due to the patch. Consider the following example: # no \into used in the script $ cat /tmp/into.sql select a from a where a = 1 \; select a+1 from a where a = 1; \set a 1 \set b 2 \set i debug(:a) \set i debug(:b) $ pgbench -r -n -t 1 -f /tmp/into.sql postgres - statement latencies in milliseconds: 2.889 select a from a where a = 1 ; 0.012 \set a 1 0.009 \set b 2 0.031 \set i debug(:a) 0.014 \set i debug(:b) # behavior wrt compound statement changes when \into is used $ cat /tmp/into.sql select a from a where a = 1 \; \into a select a+1 from a where a = 1; \into b \set i debug(:a) \set i debug(:b) $ pgbench -r -n -t 1 -f /tmp/into.sql postgres - statement latencies in milliseconds: 2.093 select a from a where a = 1 ; select a+1 from a where a = 1; 0.034 \set i debug(:a) 0.015 \set i debug(:b) One more thing I observed which I am not sure if it's a fault of this patch is illustrated below: $ cat /tmp/into.sql \; select a from a where a = 1 \; select a+1 from a where a = 1; \set a 1 \set b 2 \set i debug(:a) \set i debug(:b) $ pgbench -r -n -t 1 -f /tmp/into.sql postgres - statement latencies in milliseconds: 2.349 ; 0.013 \set a 1 0.009 \set b 2 0.029 \set i debug(:a) 0.015 \set i debug(:b) Note that the compound select statement is nowhere to be seen in the latencies output. The output remains the same even if I use the \into's. What seems to be going on is that the empty statement on the first line (\;) is the only part kept of the compound statement spanning lines 1-3. >> Also I wonder if intonames or intoargs would be a slightly better name >> for the field. > > I put "intovars" as they are variable names. Sounds fine. >> +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. Sounds fine. > >> /* >> * 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... Hmm, OK. >> +boolsql_command_in_progress = false; >> >> This variable's name could
Re: [HACKERS] pgbench - allow to store select results into variables
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: + + + Stores the first fields of the resulting row from the current or preceding + 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. +boolsql_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 options dbname + + + \into var1 [var2 ...] +
Re: [HACKERS] pgbench - allow to store select results into variables
Hi Fabien, On 2016/09/03 2:47, Fabien COELHO wrote: >> This patch needs to be rebased because of commit 64710452 (on 2016-08-19). > > Here it is! Thanks for sending the updated patch. Here are some (mostly cosmetic) comments. Before the comments, let me confirm whether the following result is odd (a bug) or whether I am just using it wrong: 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 Even the following script gives the same result: \; select a from a where a = 1; \into a \set t debug(:a) However with the following there is no error and a gets set to 2: select a from a where a = 1 \; select a+1 from a where a = 1; \into a \set t debug(:a) Comments on the patch follow: + + + Stores the first fields of the resulting row from the current or preceding + SELECT command into these variables. Any command returning rows ought to work, no? For example, the following works: update a set a = a+1 returning *; \into a \set t debug(:a) -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? 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. Also I wonder if intonames or intoargs would be a slightly better name for the field. +static bool +read_response(CState *st, char ** intos[]) Usual style seems to be to use ***intos here. +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" /* * 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? +my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1)); Need space: s/char**/char **/g This comments applies also to a couple of nearby places. -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. +boolsql_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. +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" ? Thanks, Amit -- 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 - allow to store select results into variables
Hello Amit, This patch needs to be rebased because of commit 64710452 (on 2016-08-19). Here it is! -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 285608d..a879e59 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -809,6 +809,30 @@ pgbench options dbname + + + \into var1 [var2 ...] + + + + + Stores the first fields of the resulting row from the current or preceding + SELECT 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. + + + + Example: + +SELECT abalance \into abalance + FROM pgbench_accounts WHERE aid=5432; + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 56c37d5..6433df4 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***intos; /* 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 ** intos[]) +{ + 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 (intos[compound] != NULL) + { +fprintf(stderr, + "client %d state %d compound %d: " + "cannot apply \\into to non SELECT statement\n", + st->id, st->state, compound); +st->ecnt++; +return false; + } + break; /* OK */ + + case PGRES_TUPLES_OK: + if (intos[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 (intos[compound][f] != NULL && f < nfields) +{ + /* store result as a string */ + if (!putVariable(st, "into", intos[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, intos[compound][f]); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + f++; +} + +if (intos[compound][f] != NULL) +{ + fprintf(stderr, + "client %d state %d compound %d: missing results" + " to fill into variable %s\n", + st->id, st->state, compound, intos[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
Re: [HACKERS] pgbench - allow to store select results into variables
Hi Fabien, On 2016/07/16 1:33, Fabien COELHO wrote: > Here is a v2 with more or less this approach, although \into does not end > the query, but applies to the current or last sql command. A query is > still terminated with a ";". This patch needs to be rebased because of commit 64710452 (on 2016-08-19). Thanks, Amit -- 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 - allow to store select results into variables
Hello again, I'd be okay with SELECT 1, 2 \into one two SELECT 3 \into three Here is a v2 with more or less this approach, although \into does not end the query, but applies to the current or last sql command. A query is still terminated with a ";". Now it handles things like : -- standard sql command SELECT balance FROM bank WHERE id=1; \into balance -- compound sql command, three == 3. SELECT 1, 2 \; SELECT 3 ; \into three -- compound query with 2 selects & 3 variables SELECT i \into one FROM generate_series(1, 1) AS i \; SELECT i+1, i+2 \into two three FROM generate_series(1, 1) AS i ; I had to add a few lines in psql scanner to count "\;", so the parsing logic is a little more complicated than before. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index f3afedb..cca2cc2 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -809,6 +809,30 @@ pgbench options dbname + + + \into var1 [var2 ...] + + + + + Stores the first fields of the resulting row from the current or preceding + SELECT 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. + + + + Example: + +SELECT abalance \into abalance + FROM pgbench_accounts WHERE aid=5432; + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 87fb006..667b63c 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***intos; /* per-compound command \into variables */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1148,6 +1151,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 ** intos[]) +{ + PGresult *res; + int compound = -1; + + while ((res = PQgetResult(st->con)) != NULL) + { + compound += 1; + + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + if (intos[compound] != NULL) + { +fprintf(stderr, + "client %d state %d compound %d: " + "cannot apply \\into to non SELECT statement\n", + st->id, st->state, compound); +st->ecnt++; +return false; + } + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ + break; /* OK */ + + case PGRES_TUPLES_OK: + if (intos[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 (intos[compound][f] != NULL && f < nfields) +{ + /* store result as a string */ + if (!putVariable(st, "into", intos[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, intos[compound][f]); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + f++; +} + +if (intos[compound][f] != NULL) +{ + fprintf(stderr, + "client %d state %d compound %d: missing results" + " to fill into variable %s\n", + st->id, st->state, compound, intos[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); +
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Tom, Yeah, that's seriously nasty action-at-a-distance in my view. I'd be okay with SELECT 1, 2 \into one two SELECT 3 \into three After giving it some thoughts, it could work on compound commands if \into does not close the current sql command. Something like: SELECT 1, 2 ; \into one two SELECT 3 ; \into three => 2 SQL commands SELECT 1, 2 \; \into one two SELECT 3 ; \into three => 1 compound SQL command I'd like \; or ; to stay mandatory as separators, though. Or at least to be allowed. I'm not quite sure how it could be implemented, though. And I'm with Pavel on this: it should work exactly like \gset. Hmmm. Maybe I'll do that thing in the end, but I really think than gset only makes sense in interactive context, and is pretty ugly for scripting. Inventing \into to do almost the same thing in a randomly different way exhibits a bad case of NIH syndrome. No, it is a question of design suitable to programming: > SELECT 1, 2 \gset v could not set variable "?column?" Sure, you can argue about how it's not quite the same use-case Indeed, that is my point. and so you could micro-optimize by doing it differently, No, the underlying implementation is basically the same. but that's ignoring the cognitive load on users who have to remember two different commands. I do not buy this argument: It is easier for me to remember that keyword INTO happens to do the same thing the same way in PL/pgSQL and ECPG, although with slightly different syntaxes, than to have to remember psql-specific "gset" which does the same thing but in quite a different way, because it means both another name and another concept. Claiming that plpgsql's SELECT INTO is a closer analogy than psql's \gset is quite bogus, too: I disagree. I mentionned ECPG as well. Both ECPG & PLpgSQL are "programming", psql is interactive. the environment is different (client side vs server side, ECPG is client side. I think that the side does not matter. declared vs undeclared target variables), Sure, the "gset" hack is only possible for a language without variable declarations... but that does not make it a good idea. and the syntax is different (backslash or not, commas or not, just for starters). Sure, different languages do not have the same syntax. -- 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 - allow to store select results into variables
Hello Tom, SELECT 1, 2 \; SELECT 3; \into one two three Yeah, that's seriously nasty action-at-a-distance in my view. I'd be okay with SELECT 1, 2 \into one two SELECT 3 \into three ISTM that is not the same, because then you would have two queries (over the network) instead of one, so you pay the network latency twice? but I do not think that a metacommand on a following line should retroactively affect the execution of a prior command, much less commands before the last one. Nope. The meta-command applies to the preceeding SQL command... which happens to be a \;-compound command. ISTM that all is logically fine. Some motivation about the feature (not its syntax or implementation), from a benchmarking perspective: - clients MUST read the server answers and possibly reuse them, hence a proposed \into feature. Discarding the answer as pgbench does not really comply with typical benchmark rules, eg from tpc-b: """1.3.2 Each transaction shall return to the driver the Account_Balance resulting from successful commit of the transaction. Comment: It is the intent of this clause that the account balance in the database be returned to the driver, i.e., that the application retrieve the account balance.""" - latency is important to applications (eg web applications), thus the ability to compound statements is a good thing. However, if in a bench one can compound statements but not retrieve their values, it fails the previous "retrieve the value" requirement. So basically I wish to avoid splitting compound queries and paying latency just because of a lack of syntax to do the right thing, hence the proposed feature which can retrieve data from various parts of a compound statement. -- 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 - allow to store select results into variables
On Wed, Jul 13, 2016 at 4:02 PM, Tom Lanewrote: > Robert Haas writes: > > On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO > wrote: > >> If someone thinks that "gset" is a good idea for pgbench, which I > don't, it > >> could be implemented. I think that an "into" feature, like PL/pgSQL & > ECPG, > >> makes more sense for scripting. > > > I agree: I like \into. > > > But: > > >> SELECT 1, 2 \; SELECT 3; > >> \into one two three > > > I think that's pretty weird. > > Yeah, that's seriously nasty action-at-a-distance in my view. I'd be okay > with > > SELECT 1, 2 \into one two > SELECT 3 \into three > > but I do not think that a metacommand on a following line should > retroactively affect the execution of a prior command, much less commands > before the last one. You need a test and a definition for: SELECT 1, 2; SELECT 3; \into x, y, z It should fail - "too many variables" - right? David J.
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Tom, Sending a batch of requests is a feature of libpq which is accessible through pgbench by using "\;", although the fact is not documented. It makes sense for a client to send independent queries together so as to reduce latency. You're putting an awful lot of weight on an unsupported assertion about latency. For support, I would submit that many applications today are web/mobile apps which are quite sensitive to latency. See for instance the Fast 2016 white paper by people at Google which discusses in depth "tail latency" as a key measure of quality for IO systems used for live services, or the new HTTP2 protocole (based on Google spdy) which aims at reducing latency through multiple features (compression, serveur push, pipelining...). If a user cares about that, why would she not simply merge the commands into "SELECT 1, 2, 3 \into one two three" ? Because the code would look pretty awful: SELECT (SELECT first data FROM ... JOIN ... WHERE ... ), (SELECT second data FROM ... JOIN ... WHERE ...), (SELECT third data FROM ... JOIN ... WHERE ...); And I still say that what you're proposing might be easy right now, but it might also be next door to impossible in a refactored implementation. I do not understand. There is one "multi" sql-command followed by a meta command, and somehow a refactor implementation would have troubled with that? I don't think we should go there on the basis of a weak argument about latency. \into should retrieve data only from the last PGresult. This looks pretty arbitrary: Why not the first one, as I asked for it first? Anyway, why allowing to send several queries if you are not allowed to extract their results. -- 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 - allow to store select results into variables
Fabien COELHOwrites: > Sending a batch of requests is a feature of libpq which is accessible > through pgbench by using "\;", although the fact is not documented. It > makes sense for a client to send independent queries together so as to > reduce latency. You're putting an awful lot of weight on an unsupported assertion about latency. If a user cares about that, why would she not simply merge the commands into "SELECT 1, 2, 3 \into one two three" ? And I still say that what you're proposing might be easy right now, but it might also be next door to impossible in a refactored implementation. I don't think we should go there on the basis of a weak argument about latency. \into should retrieve data only from the last PGresult. 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 - allow to store select results into variables
Hello Robert, I agree: I like \into. Great! But: SELECT 1, 2 \; SELECT 3; \into one two three I think that's pretty weird. I agree that it is weird, but I do not think that it is bad. Sending a batch of requests is a feature of libpq which is accessible through pgbench by using "\;", although the fact is not documented. It makes sense for a client to send independent queries together so as to reduce latency. From pgbench perspective, I would find it pretty weird as well that one can send several queries together but could only read the answer from... say the first one, and the others would be lost. From an implementation perspective doing it is straightforward, and rejecting it would require some more logic. An obvious nicer feature would be to allow intermixing \into & \; but ISTM that it would require to rethink deeply pgbench lexing/parsing which has just been merged with psql by Tom and others... If I had not pointed out the fact that it works, maybe no one would have noticed... so a compromise could be not to advertise the fact that it works (as the \; feature is not advertised anyway), but letting the implementation do it because it is simple and may be useful, and rephrase the documentation so that it is just about the previous select and not previous select*S*? -- 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 - allow to store select results into variables
Robert Haaswrites: > On Wed, Jul 13, 2016 at 3:02 PM, Tom Lane wrote: >> I note also that we were talking a couple months ago >> about trying to align psql and pgbench backslash commands more closely. >> This would not be a good step in that direction. > True, but I'd still argue that \into is a lot more readable than > \gset. Maybe both programs should support both commands. Meh, personal preference there no doubt. I'd be okay with both programs supporting both commands, but the \into command as described here would be quite unreasonable to implement in psql. It needs to act more like a semicolon-substitute, as \g and the rest of its family do. (I think I'd also lobby for spelling it \ginto.) 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 - allow to store select results into variables
On Wed, Jul 13, 2016 at 3:02 PM, Tom Lanewrote: > Robert Haas writes: >> On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO wrote: >>> If someone thinks that "gset" is a good idea for pgbench, which I don't, it >>> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG, >>> makes more sense for scripting. > >> I agree: I like \into. > >> But: > >>> SELECT 1, 2 \; SELECT 3; >>> \into one two three > >> I think that's pretty weird. > > Yeah, that's seriously nasty action-at-a-distance in my view. I'd be okay > with > > SELECT 1, 2 \into one two > SELECT 3 \into three > > but I do not think that a metacommand on a following line should > retroactively affect the execution of a prior command, much less commands > before the last one. Even if this happens to be easy to do in pgbench's > existing over-contorted logic, it's tremendously confusing to the user; > and it might be much less easy if we try to refactor that logic. > > And I'm with Pavel on this: it should work exactly like \gset. Inventing > \into to do almost the same thing in a randomly different way exhibits a > bad case of NIH syndrome. Sure, you can argue about how it's not quite > the same use-case and so you could micro-optimize by doing it differently, > but that's ignoring the cognitive load on users who have to remember two > different commands. Claiming that plpgsql's SELECT INTO is a closer > analogy than psql's \gset is quite bogus, too: the environment is > different (client side vs server side, declared vs undeclared target > variables), and the syntax is different (backslash or not, commas or not, > just for starters). I note also that we were talking a couple months ago > about trying to align psql and pgbench backslash commands more closely. > This would not be a good step in that direction. True, but I'd still argue that \into is a lot more readable than \gset. Maybe both programs should support both commands. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL 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 - allow to store select results into variables
Robert Haaswrites: > On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO wrote: >> If someone thinks that "gset" is a good idea for pgbench, which I don't, it >> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG, >> makes more sense for scripting. > I agree: I like \into. > But: >> SELECT 1, 2 \; SELECT 3; >> \into one two three > I think that's pretty weird. Yeah, that's seriously nasty action-at-a-distance in my view. I'd be okay with SELECT 1, 2 \into one two SELECT 3 \into three but I do not think that a metacommand on a following line should retroactively affect the execution of a prior command, much less commands before the last one. Even if this happens to be easy to do in pgbench's existing over-contorted logic, it's tremendously confusing to the user; and it might be much less easy if we try to refactor that logic. And I'm with Pavel on this: it should work exactly like \gset. Inventing \into to do almost the same thing in a randomly different way exhibits a bad case of NIH syndrome. Sure, you can argue about how it's not quite the same use-case and so you could micro-optimize by doing it differently, but that's ignoring the cognitive load on users who have to remember two different commands. Claiming that plpgsql's SELECT INTO is a closer analogy than psql's \gset is quite bogus, too: the environment is different (client side vs server side, declared vs undeclared target variables), and the syntax is different (backslash or not, commas or not, just for starters). I note also that we were talking a couple months ago about trying to align psql and pgbench backslash commands more closely. This would not be a good step in that direction. 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 - allow to store select results into variables
On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHOwrote: > If someone thinks that "gset" is a good idea for pgbench, which I don't, it > could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG, > makes more sense for scripting. I agree: I like \into. But: > SELECT 1, 2 \; SELECT 3; > \into one two three I think that's pretty weird. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL 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 - allow to store select results into variables
Also a more subjective argument: I do not like the gset automagic naming feature. I got more inspired by PL/pgSQL and ECPG which both have an "into" syntax with explicit variable names that let nothing to guessing. I like things to be simple and explicit, hence the proposed into. the gset was originally designed differently - but now it is here - and it is not practical to have two different, but pretty similar statements in psql and pgbench. In my view they are unrelated: on the one hand "gset" is really an interactive feature, where typing is costly so "automagic" might make sense; on the other hand "into" is a scripting feature, where you want to understand the code and have something as readable as possible, without surprises. The commands are named differently and behave differently. If someone thinks that "gset" is a good idea for pgbench, which I don't, it could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG, makes more sense for scripting. -- 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 - allow to store select results into variables
2016-07-09 11:19 GMT+02:00 Fabien COELHO: > > Hello Pavel, > > Why you are introducing \into and not \gset like psql does? >> > > Good question. > > The \into syntax I implemented is more generic, you can send a bunch of > queries together and extract the results, which makes sense from a client > perspective where reducing latency is important: > >SELECT 1, 2 \; SELECT 3; >\into one two three > I understand, but it looks little bit scary - but the argument of reducing latency can be valid > > However "gset" only works on the last SELECT and if all columns have a > name. This feature probably makes sense interactively, but for a script it > seems more useful to allow batch processing and collect results afterwards. > > Also a more subjective argument: I do not like the gset automagic naming > feature. I got more inspired by PL/pgSQL and ECPG which both have an "into" > syntax with explicit variable names that let nothing to guessing. I like > things to be simple and explicit, hence the proposed into. > the gset was originally designed differently - but now it is here - and it is not practical to have two different, but pretty similar statements in psql and pgbench. Regards Pavel > > -- > Fabien. >
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Pavel, Why you are introducing \into and not \gset like psql does? Good question. The \into syntax I implemented is more generic, you can send a bunch of queries together and extract the results, which makes sense from a client perspective where reducing latency is important: SELECT 1, 2 \; SELECT 3; \into one two three However "gset" only works on the last SELECT and if all columns have a name. This feature probably makes sense interactively, but for a script it seems more useful to allow batch processing and collect results afterwards. Also a more subjective argument: I do not like the gset automagic naming feature. I got more inspired by PL/pgSQL and ECPG which both have an "into" syntax with explicit variable names that let nothing to guessing. I like things to be simple and explicit, hence the proposed into. -- 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 - allow to store select results into variables
Hi 2016-07-09 10:20 GMT+02:00 Fabien COELHO: > > Hello devs, > > I mentionned my intention to add some features to pgbench back in March: > > https://www.postgresql.org/message-id/alpine.DEB.2.10.1603301618570.5677@sto > > The attached patch adds an \into meta command to store results of > preceding SELECTs into pgbench variables, so that they can be reused > afterwards. > > The feature is useful to make more realistic scripts, currently pgbench > script cannot really interact with the database as results are discarded. > > The chosen syntax is easy to understand and the implementation is quite > light, with minimal impact on the code base. I think that this is a > reasonnable compromise. > > The SELECTs must yield exactly one row, the number of variables must be > less than the number of columns. > > Also attached a set of test scripts, especially to trigger various error > cases. > > Why you are introducing \into and not \gset like psql does? Regards 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 > >
[HACKERS] pgbench - allow to store select results into variables
Hello devs, I mentionned my intention to add some features to pgbench back in March: https://www.postgresql.org/message-id/alpine.DEB.2.10.1603301618570.5677@sto The attached patch adds an \into meta command to store results of preceding SELECTs into pgbench variables, so that they can be reused afterwards. The feature is useful to make more realistic scripts, currently pgbench script cannot really interact with the database as results are discarded. The chosen syntax is easy to understand and the implementation is quite light, with minimal impact on the code base. I think that this is a reasonnable compromise. The SELECTs must yield exactly one row, the number of variables must be less than the number of columns. Also attached a set of test scripts, especially to trigger various error cases. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index f3afedb..8a7ad3e 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -809,6 +809,29 @@ pgbench options dbname + + + \into var1 [var2 ...] + + + + + Stores the first fields of the resulting row from the preceding + SELECT commands 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. + + + + Example: + +SELECT abalance FROM pgbench_accounts WHERE aid=5432; +\into balance + + + + + \set varname expression diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 87fb006..d9827dc 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -307,6 +307,7 @@ typedef struct int type; /* command type (SQL_COMMAND or META_COMMAND) */ int argc; /* number of command words */ char *argv[MAX_ARGS]; /* command word list */ + char *into[MAX_ARGS]; /* NULL-terminated target variables for \into */ PgBenchExpr *expr; /* parsed expression, if needed */ SimpleStats stats; /* time spent in this command */ } Command; @@ -1148,6 +1149,96 @@ 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 * into[]) +{ + PGresult *res; + int i = 0; + bool at_least_one = false; + + while ((res = PQgetResult(st->con)) != NULL) + { + at_least_one = true; + + switch (PQresultStatus(res)) + { + case PGRES_COMMAND_OK: /* non-SELECT commands */ + case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */ + break; /* OK */ + + case PGRES_TUPLES_OK: + if (into != NULL && into[i] != NULL) + { +/* store result into variables */ +int ntuples = PQntuples(res), + nfields = PQnfields(res), + f = 0; + +if (ntuples != 1) +{ + fprintf(stderr, + "client %d state %d expecting one row, got %d\n", + st->id, st->state, ntuples); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; +} + +while (into[i] != NULL && f < nfields) +{ + /* store result as a string */ + if (!putVariable(st, "into", into[i], PQgetvalue(res, 0, f))) + { + /* internal error, should it rather abort? */ + fprintf(stderr, +"client %d state %d: error storing into var %s\n", +st->id, st->state, into[i]); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + i++; + f++; +} + } + break; /* OK */ + + default: + /* everything else is unexpected, so probably an error */ + fprintf(stderr, "client %d aborted in state %d: %s", + st->id, st->state, PQerrorMessage(st->con)); + st->ecnt++; + PQclear(res); + discard_response(st); + return false; + } + + PQclear(res); + } + + if (!at_least_one) + { + fprintf(stderr, "client %d state %d: no results\n", st->id, st->state); + st->ecnt++; + return false; + } + + if (into != NULL && into[i] != NULL) + { + fprintf(stderr, +"client %d state %d: missing results to fill into variable %s\n", +st->id, st->state, into[i]); + 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) @@ -1764,7 +1855,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; @@ -1891,22 +1981,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