Thread: error handling in pqRowProcessor broken
The error handling for pqRowProcessor is described as
* Add the received row to the current async result (conn->result).
* Returns 1 if OK, 0 if error occurred.
*
* On error, *errmsgp can be set to an error string to be returned.
* If it is left NULL, the error is presumed to be "out of memory".
I find that this doesn't work anymore. If you set *errmsgp = "some
message" and return 0, then psql will just print a result set with zero
rows.
Bisecting points to
commit 618c16707a6d6e8f5c83ede2092975e4670201ad
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri Feb 18 15:35:15 2022 -0500
Rearrange libpq's error reporting to avoid duplicated error text.
It is very uncommon to get an error from pqRowProcessor(). To
reproduce, I inserted this code:
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index c7c48d07dc..9c1b33c6e2 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1124,6 +1124,12 @@ pqRowProcessor(PGconn *conn, const char **errmsgp)
return 0;
}
+ if (nfields == 7)
+ {
+ *errmsgp = "gotcha";
+ goto fail;
+ }
+
/*
* Basically we just allocate space in the PGresult for each field and
* copy the data over.
This will produce assorted failures in the regression tests that
illustrate the effect.
(Even before the above commit, the handling of the returned message was
a bit weird: The error output was just the message string, without any
prefix like "ERROR:".)
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> The error handling for pqRowProcessor is described as
> * Add the received row to the current async result (conn->result).
> * Returns 1 if OK, 0 if error occurred.
> *
> * On error, *errmsgp can be set to an error string to be returned.
> * If it is left NULL, the error is presumed to be "out of memory".
> I find that this doesn't work anymore.
Will look into it, thanks for reporting.
(Hmm, seems like this API spec is deficient anyway. Is the error
string to be freed later? Is it already translated?)
regards, tom lane
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> I find that this doesn't work anymore. If you set *errmsgp = "some
> message" and return 0, then psql will just print a result set with zero
> rows.
Ah, I see the problem: a few places in fe-protocol3 didn't get the memo
that conn->error_result represents a "pending" PGresult that hasn't
been constructed yet. The attached fixes it for me --- can you try it
on whatever test case led you to this?
> (Even before the above commit, the handling of the returned message was
> a bit weird: The error output was just the message string, without any
> prefix like "ERROR:".)
libpq's always acted that way for internally-generated messages.
Most of them are so rare that we're probably not used to seeing 'em.
Perhaps there's a case for making it more verbose, but right now
doesn't seem like the time to undertake that.
regards, tom lane
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 78cff4475c..1b9c04f47e 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1196,6 +1196,7 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value)
* Returns 1 if OK, 0 if error occurred.
*
* On error, *errmsgp can be set to an error string to be returned.
+ * (Such a string should already be translated via libpq_gettext().)
* If it is left NULL, the error is presumed to be "out of memory".
*
* In single-row mode, we create a new result holding just the current row,
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 94b4a448b9..e0da22dba7 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -209,7 +209,7 @@ pqParseInput3(PGconn *conn)
case 'C': /* command complete */
if (pqGets(&conn->workBuffer, conn))
return;
- if (conn->result == NULL)
+ if (conn->result == NULL && !conn->error_result)
{
conn->result = PQmakeEmptyPGresult(conn,
PGRES_COMMAND_OK);
@@ -263,7 +263,7 @@ pqParseInput3(PGconn *conn)
}
break;
case 'I': /* empty query */
- if (conn->result == NULL)
+ if (conn->result == NULL && !conn->error_result)
{
conn->result = PQmakeEmptyPGresult(conn,
PGRES_EMPTY_QUERY);
@@ -281,7 +281,7 @@ pqParseInput3(PGconn *conn)
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass == PGQUERY_PREPARE)
{
- if (conn->result == NULL)
+ if (conn->result == NULL && !conn->error_result)
{
conn->result = PQmakeEmptyPGresult(conn,
PGRES_COMMAND_OK);
@@ -362,7 +362,7 @@ pqParseInput3(PGconn *conn)
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass == PGQUERY_DESCRIBE)
{
- if (conn->result == NULL)
+ if (conn->result == NULL && !conn->error_result)
{
conn->result = PQmakeEmptyPGresult(conn,
PGRES_COMMAND_OK);
On 19.04.22 21:16, Tom Lane wrote: > Peter Eisentraut<peter.eisentraut@enterprisedb.com> writes: >> I find that this doesn't work anymore. If you set *errmsgp = "some >> message" and return 0, then psql will just print a result set with zero >> rows. > Ah, I see the problem: a few places in fe-protocol3 didn't get the memo > that conn->error_result represents a "pending" PGresult that hasn't > been constructed yet. The attached fixes it for me --- can you try it > on whatever test case led you to this? Your patch fixes it for me.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 19.04.22 21:16, Tom Lane wrote:
>> Ah, I see the problem: a few places in fe-protocol3 didn't get the memo
>> that conn->error_result represents a "pending" PGresult that hasn't
>> been constructed yet. The attached fixes it for me --- can you try it
>> on whatever test case led you to this?
> Your patch fixes it for me.
Thanks for testing. I pushed a cosmetically-polished version of that.
regards, tom lane