Hello 2012/9/19 Shigeru HANADA <shigeru.han...@gmail.com>: > On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule <pavel.steh...@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. >
I use a same "SetVariable" function, so a behave should be same > 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 I invite any proposals about enhancing documentation. Personally I am a PostgreSQL developer, so I don't known any different term other than "target list" - but any user friendly description is welcome. > > 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. yes - it is question. I use same pattern like PrintQueryResult, but "bad response" message should be used. I am sending updated patch > > Although I'll look the code more closely later, but anyway I marked the > patch "Waiting on Author" for comments above. > > Regards, > -- > Shigeru HANADA
gset_04.diff
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
