Re: pgbench - allow to store select results into variables

Hi Fabien,

On 2016/09/13 17:41, Fabien COELHO wrote:
>
> Hello Amit,
>
>> [...]
>> There still seems to be a change in behavior of the -r option due to the
>> patch. Consider the following example:
>>
>> select a from a where a = 1 \;
>> select a+1 from a where a = 1;
>> ...
>> - statement latencies in milliseconds:
>> 2.889 select a from a where a = 1 ;
>
> vs
>
>> select a from a where a = 1 \; \into a
>> select a+1 from a where a = 1; \into b
>> ...
>> 2.093 select a from a where a = 1 ; select a+1 from a where a = 1;
>
> Yep.
>
> Note that there is a small logical conumdrum in this argument: As the
> script is not the same, especially as into was not possible before,
> strictly speaking there is no behavior "change".

Sure, scripts are not the same but it seemed like showing the whole
compound query whereas previously only part of it was shown may be an
implementation artifact of \into.

> This said, what you suggest can be done.
>
> After giving it some thought, I suggest that it is not needed nor
> desirable. If you want to achieve the initial effect, you just have to put
> the "into a" on the next line:
>
> select a from a where a = 1 \;
> \into a
> select a+1 from a where a = 1; \into b
>
> Then you would get the -r cut at the end of the compound command. Thus the
> current version gives full control of what will appear in the summary. If
> I change "\into xxx\n" to mean "also cut here", then there is less control
> on when the cut occurs when into is used.

So it means that position of \into affects where a compound command gets
cut for -r display. I was just wondering if that was unintentional.

>> 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;
>>
>> $ pgbench -r -n -t 1 -f /tmp/into.sql postgres
>> <snip>
>> - statement latencies in milliseconds:
>> 2.349 ;
>>
>> 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.
>
> Yes.
>
> This is really the (debatable) current behavior, and is not affected by
> the patch. The "-r" summary takes the first line of the command, whatever
> it is. In your example the first line is "\;", so you get what you asked
> for, even if it looks rather strange, obviously.

Yep, perhaps it's strange to write a script like that anyway, :)

>>>> + bool sql_command_in_progress = false;
>> [...]
>> 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.
>
> Ok. I changed it "sql_command_lexing_in_progress".
>
>>> 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.
>
> I have considered it. As the legitimate behavior you suggested can be
> achieved just by putting the into on the next line, ISTM that the current
> proposition gives more control than doing a mandatory cut when into is used.
>
> Attached is a new version with the boolean renaming.

Thanks.

> The other thing I have considered is whether to implemented a "\gset"
> syntax, as suggested by Pavel and Tom. Bar the aesthetic, the main issue I
> have with it is that it does not work with compound commands, and what I
> want is to get the values out of compound commands... because of my focus
> on latency... so basically "\gset" does not do the job I want... Now I
> recognize that other people would like it, so probably I'll do it anyway
> in another patch.

So, psql's \gset does not work as desired for compound commands (viz. it
is only able to save the result of the last sub-command). You need to use
\gset with each sub-command separately if no result should be discarded.
Because of undesirable latency characteristics of sending multiple
commands, you want to be able to modify compound command handling such
that every sub-command's result could be saved. In that regard, it's good
that pgbench does not use PQexec() which only returns the result of the
last sub-command if a compound command was issued through it. pgbench's
doCustom() currently discards all results by issuing discard_response().
You propose to change things such that results are intercepted in a new
function read_response(), values assigned to intovars corresponding to
each sub-command, and then discarded. The intovars arrays are allocated
within each sub-command's Command struct when parsing the compound command
based on where \into is located wrt to the sub-command.

So, most of the code in the patch is about handling compound statements to
be be able to save results of all sub-commands, not just the last. Do you
think it would be OK to suffer the bad latency of multiple round trips and
implement a version of \into (or \gset or \ginto) for pgbench scripts that
behaves exactly like psql's \gset as a first step? But you say you will
do it as another patch.

By the way, I tend to agree with your point about \gset syntax being
suitable (only) in a interactive context such as psql; it's not as
readable as \into x y ... when used in a script.

Thanks,
Amit