From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Phil Sorber <phil(at)omniti(dot)com> |
Cc: | Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Erik Rijkers <er(at)xs4all(dot)nl>, David Fetter <david(at)fetter(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: proposal - assign result of query to psql variable |
Date: | 2012-10-27 22:16:39 |
Message-ID: | CAFj8pRBHQS0XcpKP-fAnqcV0Gch9ejVzRmvrYzKg3n=EDb7eiQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
Attachment | Content-Type | Size |
---|---|---|
gset_12.diff | application/octet-stream | 18.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Sabino Mullane | 2012-10-27 23:15:14 | Re: My first patch! (to \df output) |
Previous Message | Gavin Flower | 2012-10-27 21:12:59 | Re: Logical to physical page mapping |