Re: pgbench - allow to store select results into variables

Hi Fabien,

On 2016/09/03 2:47, Fabien COELHO wrote:
>> This patch needs to be rebased because of commit 64710452 (on 2016-08-19).
> 
> Here it is!

Thanks for sending the updated patch. Here are some (mostly cosmetic)
comments.  Before the comments, let me confirm whether the following
result is odd (a bug) or whether I am just using it wrong:

Custom script looks like:

\;
select a \into a   from tab where a = 1;
\set i debug(:a)

I get the following error:

undefined variable "a"
client 0 aborted in state 1; execution of meta-command failed

Even the following script gives the same result:

\;
select a from a where a = 1;
\into a
\set t debug(:a)

However with the following there is no error and a gets set to 2:

select a from a where a = 1
\;
select a+1 from a where a = 1;
\into a
\set t debug(:a)


Comments on the patch follow:

+    <listitem>
+     <para>
+      Stores the first fields of the resulting row from the current or
preceding
+      <command>SELECT</> command into these variables.

Any command returning rows ought to work, no?  For example, the following
works:

update a set a = a+1 returning *;
\into a
\set t debug(:a)


-    char       *line;           /* text of command line */
+    char       *line;           /* first line for short display */
+    char       *lines;          /* full multi-line text of command */

When I looked at this and related hunks (and also the hunks related to
semicolons), it made me wonder whether this patch contains two logical
changes.  Is this a just a refactoring for the \into implementation or
does this provide some new externally visible feature?

    char       *argv[MAX_ARGS]; /* command word list */
+    int         compound;      /* last compound command (number of \;) */
+    char    ***intos;          /* per-compound command \into variables */

Need an extra space for intos to align with earlier fields.  Also I wonder
if intonames or intoargs would be a slightly better name for the field.


+static bool
+read_response(CState *st, char ** intos[])

Usual style seems to be to use ***intos here.


+                fprintf(stderr,
+                        "client %d state %d compound %d: "
+                        "cannot apply \\into to non SELECT statement\n",
+                        st->id, st->state, compound);

How about make this error message say something like other messages
related to \into, perhaps something like: "\\into cannot follow non SELECT
statements\n"

            /*             * Read and discard the query result; note this is not
included in
-             * the statement latency numbers.
+             * the statement latency numbers (above), thus if reading the
+             * response fails the transaction is counted nevertheless.             */

Does this comment need to mention that the result is not discarded when
\into is specified?


+    my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1));

Need space: s/char**/char **/g

This comments applies also to a couple of nearby places.


-        my_command->line = pg_malloc(nlpos - p + 1);
+        my_command->line = pg_malloc(nlpos - p  + 1 + 3);

A comment mentioning what the above means would be helpful.


+    bool        sql_command_in_progress = false;

This variable's name could be slightly confusing to readers. If I
understand its purpose correctly, perhaps it could be called
sql_command_continues.


+                    if (index == 0)
+                        syntax_error(desc, lineno, NULL, NULL,
+                                     "\\into cannot start a script",
+                                     NULL, -1);

How about: "\\into cannot be at the beginning of a script" ?

Thanks,
Amit