Re: pgbench - allow to store select results into variables

Hi Fabien,

On 2016/09/07 23:01, Fabien COELHO wrote:
>> 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
> 
> Good catch!
> 
> Indeed, there is a problem with empty commands which are simply ignored by
> libpq/postgres if there are other commands around, so my synchronization
> between results & commands was too naive.
> 
> In order to fix this, I made the scanner also count empty commands and
> ignore comments. I guessed that proposing to change libpq/postgres
> behavior was not an option.

Seems to be fixed.

>> 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?
> 
> Yes. I put "SQL command" instead.

Check.

>> -    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?
> 
> There is essentially a refactoring that I did when updating how Command is
> managed because it has to be done in several stages to fit "into" into it
> and to take care of compounds.
> 
> However there was small "new externally visible feature": on -r, instead
> of cutting abruptly a multiline command at the end of the first line it
> appended "..." as an ellipsis because it looked nicer.
> I've removed this small visible changed.

There still seems to be a change in behavior of the -r option due to the
patch. Consider the following example:

# no \into used in the script
$ cat /tmp/into.sql
select a from a where a = 1 \;
select a+1 from a where a = 1;
\set a 1
\set b 2
\set i debug(:a)
\set i debug(:b)

$ pgbench -r -n -t 1 -f /tmp/into.sql postgres
<snip>- statement latencies in milliseconds:        2.889  select a from a where a = 1 ;        0.012  \set a 1
0.009 \set b 2        0.031  \set i debug(:a)        0.014  \set i debug(:b)
 

# behavior wrt compound statement changes when \into is used
$ cat /tmp/into.sql
select a from a where a = 1 \; \into a
select a+1 from a where a = 1; \into b
\set i debug(:a)
\set i debug(:b)

$ pgbench -r -n -t 1 -f /tmp/into.sql postgres
<snip>- statement latencies in milliseconds:        2.093  select a from a where a = 1 ; select a+1 from a where a = 1;
      0.034  \set i debug(:a)        0.015  \set i debug(:b)
 

One more thing I observed which I am not sure if it's a fault of this
patch is illustrated below:

$ cat /tmp/into.sql
\;
select a from a where a = 1 \;
select a+1 from a where a = 1;
\set a 1
\set b 2
\set i debug(:a)
\set i debug(:b)

$ pgbench -r -n -t 1 -f /tmp/into.sql postgres
<snip>- statement latencies in milliseconds:        2.349  ;        0.013  \set a 1        0.009  \set b 2        0.029
\set i debug(:a)        0.015  \set i debug(:b)
 

Note that the compound select statement is nowhere to be seen in the
latencies output. The output remains the same even if I use the \into's.
What seems to be going on is that the empty statement on the first line
(\;) is the only part kept of the compound statement spanning lines 1-3.

>> Also I wonder if intonames or intoargs would be a slightly better name
>> for the field.
> 
> I put "intovars" as they are variable names.

Sounds fine.

>> +                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"
> 
> As you pointed out above, there may be statements without "SELECT" which
> which return a row. I wrote "\\into expects a row" instead.

Sounds fine.

> 
>>             /*
>>              * 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?
> 
> Hmmm. The result structure is discarded, but the results are copied into
> variables before that, so the comments seems ok...

Hmm, OK.

>> +    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.
> 
> It is possible. I like 'in progress' though. Why is it confusing?
> It means that the current command is not finished yet and more is
> expected, that is the final ';' has not been encountered.

I understood that it refers to what you explain here.  But to me it
sounded like the name is referring to the progress of *execution* of a SQL
command whereas the code in question is simply expecting to finish
*parsing* the SQL command using the next lines.  It may be fine though.

>> +                    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" ?
> 
> It is possible, but it's quite longer... I'm not a native speaker, so I'm
> do not know whether it would be better.

Me neither, let's leave it for the committer to decide.

> The attached patch takes into all your comments but:
>  - comment about discarded results...
>  - the sql_command_in_progress variable name change
>  - the longer message on into at the start of a script

The patch seems fine without these, although please consider the concern I
raised with regard to the -r option above.

Thanks,
Amit