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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers