Thread: libpq PQresultErrorMessage vs PQerrorMessage API issue
Hi all
This morning for the the umpteenth time I saw:
some error message: [blank here]
output from a libpq program.
That's because passing a NULL PGresult to PQgetResultErrorMessage() returns "". But a NULL PGresult is a normal result from PQexec when it fails to submit a query due to an invalid connection, when PQgetResult can't get a result from an invalid connection, etc.
E.g. this pattern:
res = PQexec(conn, "something");
if (PQstatus(res) != PGRES_TUPLES_OK)
{
report_an_error("some error message: %s",
PQresultErrorMessage(res));
}
... where "res" is NULL because the connection was invalid, so PQresultErrorMessage(res) emits the empty string.
As a result, using PQerrorMessage(conn) is actually better in most cases, despite the static error buffer issues. It'll do the right thing when the connection itself is bad. Alternately you land up with the pattern
res == NULL ? PQerrorMessage(conn) : PQresultErrorMessage(res)
I'm not quite sure what to do about this. Ideally PQresultErrorMessage() would take the PGconn* too, but it doesn't, and it's not too friendly to add an extra argument. Plus arguably they mean different things.
Maybe it's as simple as changing the docs to say that you should prefer PQerrorMessage() if you aren't using multiple PGresult s at a time, and mentioning the need to copy the error string.
But I'd kind of like to instead return a new non-null PGresult PGRES_RESULT_ERROR whenever we'd currently return a NULL PGresult due to a failure. Embed the error message into the PGresult, so PQresultErrorMessage() can fetch it. Because a PGresult needs to be owned by a PGconn and a NULL PGconn can't own anything, we'd instead return a pointer to a static const global PGresult with value PGRES_NO_CONNECTION if any function is passed a NULL PGconn*. That way it doesn't matter if it gets PQclear()ed or not. And PQclear() could test for (res == PGresult_no_connection) and not try to free it if found.
The main issue I see there is that existing code may expect a NULL PGresult and may test for (res == NULL) as an indicator of a query-sending failure from PQexec etc. So I suspect we'd need a libpq-global option to enable this behaviour for apps that are aware of it - we wouldn't want to add new function signature variants after all.
Craig Ringer <craig.ringer@enterprisedb.com> writes:
> This morning for the the umpteenth time I saw:
> some error message: [blank here]
> output from a libpq program.
> That's because passing a NULL PGresult to PQgetResultErrorMessage() returns
> "". But a NULL PGresult is a normal result from PQexec when it fails to
> submit a query due to an invalid connection, when PQgetResult can't get a
> result from an invalid connection, etc.
How much of this is due to programmers not bothering to check whether
PQconnectXXX succeeded? I do not think we need to go far out of our
way to cope with that scenario.
The idea of having a static PGresult that we can hand back to denote
out-of-memory scenarios is kind of cute. But again, I wonder how
often the situation comes up in the real world. It might be worth
doing just to have a more consistent API spec, though. Particularly
for PQgetResult, where a NULL result has a defined, non-error meaning.
In general I doubt there's enough of a problem here to justify
inventing new or different APIs. But if we can sand down some
rough edges without doing that, let's have a look.
regards, tom lane