Re: proposal - assign result of query to psql variable

On Tue, Oct 16, 2012 at 12:24 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2012/10/16 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> Hi Pavel,
>>
>> On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>>> here is updated patch, I moved lot of code from lexer to command.com,
>>> and now more \gset doesn't disable other backslash commands on same
>>> line.
>>
>> * lexer changes
>> IIUC, new function psql_scan_varlist_varname scans input and returns a
>> variable name or a comma at each call, and command.c handles the error
>> such as invalid # of variables. This new design seems better than old one.
>>
>> However, IMHO the name psql_scan_varlist_varname sounds redundant and
>> unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
>> that that function is expected to be used for arguments of backslash
>> commands, like psql_scan_slash_command and psql_scan_slash_option.
>> Thoughts?
>>
>> * multiple meta command
>> Now both of the command sequences
>>
>> $ SELECT 1, 2 \gset var1, var2 \g foo.txt
>> $ SELECT 1, 2 \g foo.txt \gset var1, var2
>>
>> set var1 and v2 to "1" and "2" respectively, and also write the result
>> into foo.txt. This would be what users expected.
>>
>> * Duplication of variables
>> I found an issue we have not discussed. Currently \gset accepts same
>> variable names in the list, and stores last SELECT item in duplicated
>> variables. For instance,
>>
>> $ SELECT 1, 2 \gset var, var
>>
>> stores "2" into var. I think this behavior is acceptable, but it might
>> be worth mentioning in document.
>>
>> * extra fixes
>> I fixed some minor issues below. Please see attached v10 patch for details.
>>
>> * remove unused macro OT_VARLIST
>> * remove unnecessary #include directive for common.h
>> * fill comment within 80 columns
>> * indent short variable name with tab
>> * add regression test case for combination of \g and \gset
>>
>> * bug on FETCH_COUNT = 1
>> When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
>> \gset shows extra "(1 row)". This would be a bug in
>> ExecQueryUsingCursor. Please see the last test case in regression test
>> psql_cmd.
>
> I fixed this bug
>
> Regards
>
> Pavel
>
>>
>> I'll mark this patch as "waiting author".
>>
>> Regards,
>> --
>> Shigeru HANADA
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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.

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

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.

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.

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?

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.