From: Stephen Frost <sfrost@snowman.net>
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: PostgreSQL Developers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] pgbench - allow to store select results into variables
Date: Sat, 7 Apr 2018 20:00:01 -0400
Message-ID: <20180408000001.GD27724@tamriel.snowman.net> (raw)
In-Reply-To: <alpine.DEB.2.20.1804071734480.4721@lancre>
References: <alpine.DEB.2.20.1708132029250.15606@lancre>
<CAFj8pRDukEVowiGddVs9P9oNUrKui8pRm29kJUjZgG-ZjcCN=Q@mail.gmail.com>
<alpine.DEB.2.20.1709081916410.3568@lancre>
<alpine.DEB.2.20.1710201836530.15170@lancre>
<CAFj8pRCGNRALRm6r2n-9iZQENJ92J3Hm=17mwXKeERXuNuAq7g@mail.gmail.com>
<alpine.DEB.2.20.1710251209330.24229@lancre>
<alpine.DEB.2.20.1801100952040.2283@lancre>
<alpine.DEB.2.20.1803241024500.13537@lancre>
<20180407031725.GX27724@tamriel.snowman.net>
<alpine.DEB.2.20.1804071734480.4721@lancre>
Fabien,
* Fabien COELHO (coelho@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
Attachments:
[application/pgp-signature] signature.asc (818B, ../20180408000001.GD27724@tamriel.snowman.net/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, coelho@cri.ensmp.fr
Subject: Re: [HACKERS] pgbench - allow to store select results into variables
In-Reply-To: <20180408000001.GD27724@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox