Re: proposal - assign result of query to psql variable - Mailing list pgsql-hackers

Hello

here is updated patch

main change - it doesn't touch psql lexer - like Tom proposed
other points respect Phil's notices


>>
>
> My first review...
>
> Patch applied cleanly to master and make check was fine.
>
> I tested it out and it operates as advertised. There were a couple
> things that stood out to me though.
>
> 1) NULL values are not displayed properly after \pset null is run.
>
> postgres=# \pset null '(null)'
> Null display is "(null)".
> postgres=# select NULL \gset var1
> postgres=# \echo :var1
>
> postgres=# select NULL;
>  ?column?
> ----------
>  (null)
> (1 row)
>
> I know this doesn't come back from the server like this, but you
> should be able to pull this from the options and display
> appropriately. Not sure if it should be in variable display code, or
> when you store it into the variable.

fixed and add to regress test

>
> 2) The error messages seemed kind of terse. Other error messages are
> capitalized and have punctuation.

After some thinking I didn't change it - it is consistent with other
messages in psql - short messages that are not complete sentence are
no capitalized and have not punctuation like other short messages in
psql

>
> 3) We don't find out about incorrect number of columns until after
> query returns. I know this is hard/impossible to fix, but it might be
> nice to print out the result normally if you can't store it in the
> variables.

I didn't change it because a) I don't think so change behave after
error is good idea, b) \gset doesn't remove SQL from query buffer, so
you can repeat it

postgres=> select 10,20,30
postgres-> \gset a,b
too few target variables
postgres=> \g
 ?column? │ ?column? │ ?column?
──────────┼──────────┼──────────
       10 │       20 │       30
(1 row)

postgres=> \gset a,b,c


>
> 3b) You throw an error on too many variables, but still store the data
> since you have fewer columns than variables. This makes sense, but you
> don't inform the user at all.

it is commented in doc

+         <para>
+          When this command fails, then related <replaceable
+          class="parameter">variables</replaceable> has undefined content.
+         </para>

>
> On to the code:
>
> 1) Introduction of random newlines:
>
> *************** cleanup:
> *** 1254,1259 ****
> --- 1383,1389 ----
>                 PQclear(results);
>         }
>
> +
>         if (pset.timing)
>         {
>                 INSTR_TIME_SET_CURRENT(after);
>
> I saw that in a couple places, but that was the most obvious.
>

I hope so I moved to /dev/null all

> 2) TargetList - Why not use the built in linked list operations rather
> than creating your own? Are they not accessible to client binaries
> like this?

There was not support for lists on client part to today. So I had to
created own simple implementation.

>
> Overall I think this is a useful feature and I think you integrated it
> well within the existing infrastructure, ie combining concepts of \set
> and \g.

Thank you very much again

Regards

Pavel