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. 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
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 0e9b408..a76b84d 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -832,7 +832,7 @@ PrintQueryResults(PGresult *results) * * Note: Utility function for use by SendQuery() only. * - * Returns true if the query executed sucessfully, false otherwise. + * Returns true if the query executed successfully, false otherwise. */ static bool StoreQueryResult(PGresult *result) @@ -865,7 +865,7 @@ StoreQueryResult(PGresult *result) { if (!iter) { - psql_error("to few target variables\n"); + psql_error("too few target variables\n"); success = false; break; } @@ -902,7 +902,7 @@ StoreQueryResult(PGresult *result) case PGRES_COPY_OUT: case PGRES_COPY_IN: - psql_error("COPY isnot supported by \\gset command\n"); + psql_error("COPY is not supported by \\gset command\n"); success = false; break; @@ -1797,7 +1797,7 @@ expand_tilde(char **filename) /* - * Add name of internal variable to query targer list + * Add name of internal variable to query target list * */ TargetList diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 90ab9bd..54fa490 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -15,7 +15,7 @@ select 10, 20, 'Hello World' -- should fail \gset ,, \gset , -to few target variables +too few target variables \gset ,,, too many target variables -- should be ok
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers