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
