Re: [HACKERS] pgbench - allow to store select results into variables - Mailing list pgsql-hackers

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