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

Hello Stephen,

> Here's that review.

Thanks for the review.

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

Dunno.

I put them there because it is in alphabetical order (for cset at least)
and because "set" documentation is heavy about expressions (operators,
functions, constants, ...) which have nothing to do with cset/gset, so I
felt that having them clearly separated and in abc order was best.

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

Indeed, looks better.

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

I added a (shorter) comment.

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

It could match PL/pgSQL's INTO, that is first row or NULL if none.

>> +
>> + /* store result as a string */
>> + if (!putVariable(st, "gset", varname,
>> + PQgetvalue(res, 0, f)))
>> + {
>> + /* internal error, should it rather abort? */
>
> 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.

I'm also still thinking about it:-) For me it is an abort, but there is
this whole "return false" internal error management in pgbench the purpose
of which fails me a little bit, so I stick to that anyway.

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

I added two comments to avoid a malloc/free when there are no prefixes.
ISTM that although it might be a border-line over-optimization case, it is
a short one, the free is a few lines away, so it might be ok.

>> + /* read and discard the query results */
>
> That comment doesn't feel quite right now. ;)

Indeed. Changed with "store or discard".

>>
>> - /* 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.

Indeed.

>> + /* merge gset variants into preceeding SQL command */
>> + if (pg_strcasecmp(bs_cmd, "gset") == 0 ||
>> + pg_strcasecmp(bs_cmd, "cset") == 0)
>> + {
>> + "\\gset cannot start a script",
>> + "\\gset must follow a SQL command",
>> + "\\gset cannot follow one another",
>
> These errors should probably be '\\gset and \\cset' or similar, no?
> Since we fall into this for both..

Indeed.

Attached an improved version, mostly comments and error message fixes.

I have not changed the 1 row constraint for now. Could be an later
extension.

If this patch get through, might be handy for simplifying tests in
current pgbench submissions, especially the "error handling" one.

--
Fabien.