On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:
> there is new version of this patch
>
> * cleaned var list parser
> * new regress tests
> * support FETCH_COUNT > 0
Here are my review comments.
Submission
==========
The patch is formatted in context diff style, and it could be applied
cleanly against latest master. This patch include document and tests,
but IMO they need some enhancement.
Usability
=========
This patch provides new psql command \gset which sends content of query
buffer to server, and stores result of the query into psql variables.
The name "\gset" is mixture of \g, which sends result to file or pipe,
and \set, which sets variable to some value, so it would sound natural
to psql users.
Freature test
=============
Compile completed without warning. Regression tests for \gset passed,
but I have some comments on them.
- Other regression tests have comment "-- ERROR" just after queries
which should fail. It would be nice to follow this manner.
- Typo "to few" in expected file and source file.
- How about adding testing "\gset" (no variable list) to "should fail"?
- Is it intentional that \gset can set special variables such as
AUTOCOMMIT and HOST? I don't see any downside for this behavior,
because \set also can do that, but it is not documented nor tested at all.
Document
========
- Adding some description of \gset command, especially about limitation
of variable list, seems necessary.
- In addition to the meta-command section, "Advanced features" section
mentions how to set psql's variables, so we would need some mention
there too.
- The term "target list" might not be familiar to users, since it
appears in only sections mentioning PG internal relatively. I think
that the feature described in the section "Retrieving Query Results" in
ECPG document is similar to this feature.
http://www.postgresql.org/docs/devel/static/ecpg-variables.html
Coding
======
The code follows our coding conventions. Here are comments for coding.
- Some typo found in comments, please see attached patch.
- There is a code path which doesn't print error message even if libpq
reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
prints "bad response" message for those errors.
Although I'll look the code more closely later, but anyway I marked the
patch "Waiting on Author" for comments above.
Regards,
--
Shigeru HANADA
