Re: [HACKERS] pgbench - allow to store select results into variables

Fabien,

* Fabien COELHO (fabien(dot)coelho(at)mines-paristech(dot)fr) wrote:
> Patch v16 is a rebase.

Here's that review.

> diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
> index d52d324..203b6bc 100644
> --- a/doc/src/sgml/ref/pgbench.sgml
> +++ b/doc/src/sgml/ref/pgbench.sgml
> @@ -900,6 +900,51 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
> </para>
>
> <variablelist>
> + <varlistentry id='pgbench-metacommand-gset'>
> + <term>
> + <literal>\cset [<replaceable>prefix</replaceable>]</literal> or
> + <literal>\gset [<replaceable>prefix</replaceable>]</literal>
> + </term>

Seems like this should really be moved down below the section for
'\set'.

> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index 894571e..4a8595f 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -434,12 +434,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 */

Not really a fan of such closely-named variables... Maybe first_line
instead?

> +/* 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)

Probably be good to add some comments here, eh:

/*
* The results are intentionally thrown away if we aren't under a gset
* call.
*/

> + {
> + /* 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;
> + }

Might be interesting to support mutli-row (or no row?) returns in the
future, but not something this patch needs to do, so this looks fine to
me.

> + 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;
> + }

I'm a bit on the fence about if we should abort in this case or not. A
failure here seems likely to happen consistently (whereas the ntuples
case might be a fluke of some kind), which tends to make me think we
should abort, but still thinking about it.

> + if (*gset[compound] != '\0')
> + free(varname);

A comment here, and above where we're assigning the result of the
psprintf(), to varname probably wouldn't hurt, explaining that the
variable is sometimes pointing into the query result structure and
sometimes not...

Thinking about it a bit more, wouldn't it be cleaner to just always use
psprintf()? eg:

char *varname;

varname = psprintf("%s%s", gset[compound] != '\0' ? gset[compound] : "", varname);

...

free(varname);

> + /* read and discard the query results */

That comment doesn't feel quite right now. ;)

> @@ -3824,8 +3910,7 @@ parseQuery(Command *cmd)
> char *sql,
> *p;
>
> - /* We don't want to scribble on cmd->argv[0] until done */
> - sql = pg_strdup(cmd->argv[0]);
> + sql = pg_strdup(cmd->lines);

The function-header comment for parseQuery() could really stand to be
improved.

> + /* merge gset variants into preceeding SQL command */
> + if (pg_strcasecmp(bs_cmd, "gset") == 0 ||
> + pg_strcasecmp(bs_cmd, "cset") == 0)
> + {
> + int cindex;
> + Command *sql_cmd;
> +
> + is_compound = bs_cmd[0] == 'c';
> +
> + if (index == 0)
> + syntax_error(desc, lineno, NULL, NULL,
> + "\\gset cannot start a script",
> + NULL, -1);
> +
> + sql_cmd = ps.commands[index-1];
> +
> + if (sql_cmd->type != SQL_COMMAND)
> + syntax_error(desc, lineno, NULL, NULL,
> + "\\gset must follow a SQL command",
> + sql_cmd->line, -1);
> +
> + /* this \gset applies to the last sub-command */
> + cindex = sql_cmd->compound;
> +
> + if (sql_cmd->gset[cindex] != NULL)
> + syntax_error(desc, lineno, NULL, NULL,
> + "\\gset cannot follow one another",
> + NULL, -1);
> +
> + /* get variable prefix */
> + if (command->argc <= 1 || command->argv[1][0] == '\0')
> + sql_cmd->gset[cindex] = "";
> + else
> + sql_cmd->gset[cindex] = command->argv[1];
> +
> + /* cleanup unused backslash command */
> + pg_free(command);

These errors should probably be '\\gset and \\cset' or similar, no?
Since we fall into this for both.. Probably not a big deal to someone
using pgbench, but still.

So, overall, looks pretty good to me. There's definitely some cleanup
work to be done with variable names and comments and such, but nothing
too terrible and I should have time to go through those changes and then
go back over the patch again tomorrow with an eye towards committing it
tomorrow afternoon, barring objections, etc.

Thanks!

Stephen