libpq PQresultErrorMessage vs PQerrorMessage API issue

Started by Craig Ringeralmost 5 years ago2 messages
#1Craig Ringer
craig.ringer@enterprisedb.com

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.

Similar changes would make sense for returning NULL when there are no
result sets remaining after a PQsendQuery, and for returning NULL after
row-by-row fetch mode runs out of rows.

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Craig Ringer (#1)
Re: libpq PQresultErrorMessage vs PQerrorMessage API issue

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