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 17:23:38 |
Message-ID: | CAFj8pRCVPcjnCQFp8D9ZsxgGqdK9nSsGCbCDmej+goLsHTaqfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello
>
> 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.
ok
>
> 2) The error messages seemed kind of terse. Other error messages are
> capitalized and have punctuation.
ok
>
> 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.
a changing behave when error is occurred is not good, but I can show a
number of returned columns
>
> 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.
there is not a some like stack, so I cannot to return values to
previous state. It should be documented - after error, a related
variables has undefined values.
>
> 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.
>
> 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?
actually, there are no support for lists on client side now. So it is
reason. But I'll remove it, because I'll move parsing to command
implementation, and then I don't need a list support
>
> 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
Regards
Pavel
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2012-10-27 18:04:01 | Re: Performance Improvement by reducing WAL for Update Operation |
Previous Message | Pavel Stehule | 2012-10-27 15:45:54 | Re: My first patch! (to \df output) |