libpq PQresultErrorMessage vs PQerrorMessage API issue

From: Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>
To: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Subject: libpq PQresultErrorMessage vs PQerrorMessage API issue
Date: 2021-02-16 06:29:30
Message-ID: CAGRY4nwVP-z36_r2P-Sk+3kr6PHaOe+rmiGZy8dKyWgtLfboeA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Justin Pryzby 2021-02-16 06:42:14 progress reporting for partitioned REINDEX
Previous Message Amit Langote 2021-02-16 06:05:50 Re: A reloption for partitioned tables - parallel_workers