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

Fabien,

* Fabien COELHO (coelho(at)cri(dot)ensmp(dot)fr) wrote:
> >> <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.

Ah, hmmm, yes, alphabetical order is sensible, certainly.

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

Great, thanks.

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

Ok.

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

Yeah, but that's not really something that needs to go into this patch.

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

Yeah.

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

Ok, having the comments is definitely good as it was a bit confusing as
to what was going on. :)

> >>+ /* read and discard the query results */
> >
> >That comment doesn't feel quite right now. ;)
>
> Indeed. Changed with "store or discard".

Ok.

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

Glad to hear that. Unfortunately, I didn't end up having time to wrap
this up to a satisfactory level for myself to get it into PG11. I know
it's been a long time coming, and thank you for continuing to push on
it; I'll try to make sure that I take some time in the first CF for PG12
to wrap this up and get it in. I'm all for these improvements in
pgbench in general, it's a really useful tool and it's great to see work
going into it.

Thanks again!

Stephen