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

Hello Amit,

> 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.

> 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.

>
> -    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.

>     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.

Ok.

> Also I wonder if intonames or intoargs would be a slightly better name
> for the field.

I put "intovars" as they are variable names.

> +static bool
> +read_response(CState *st, char ** intos[])
>
> Usual style seems to be to use ***intos here.

Ok.

> +                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.

>             /*
>              * 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...

> +    my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1));
>
> Need space: s/char**/char **/g

Ok.

> This comments applies also to a couple of nearby places.

Indeed.

> -        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.

Ok. I removed the "+ 3" anyway.

> +    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.

> +                    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.

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

--
Fabien.