Fabien, * Fabien COELHO (coe...@cri.ensmp.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
signature.asc
Description: PGP signature
