NB: This posting is a copy of
https://rt.cpan.org/Public/Bug/Display.html?id=66792
intended to enable 'easier' discussion of the proposed change.
Whenever a libpq function whose status cannot be queried by examining a
PGresult fails, the driver just reports a 'fatal error' to an
application using it without providing more specific information via
imp_dbh->sqlstate. This is unfortunate for long running applications
manageing persistent database connection because these cannot
distinguish between 'unexpected errors' (possibly caused by programming
errors in the application) and TCP connection failures which can 'just
happen' on the internet, where an automated remedy (try to re-establish
the connection perdiodically until success) is easily available. The
driver itself can detect this by examining the connection status via
PQconnStatus call.
Attached is a patch which (IMHO) improves this situation by introducing
a _fatal_sqlstate function setting the sqlstate of the dbh either to
08000 (connection exception) or 22000 (data exception), based on the
result of PQconnStatus. Calls to this function have been inserted in
front of all pg_error invocations happening because of some kind of
libpq error return. It also changes the error handling in pg_db_cancel
to set the sqlstate via _sqlstate before calling pg_error. I've also
taken the liberty to streamline the _sqlstate function somewhat, by
dropping the technically redundant stateset variable and consolidiating
the various strncpy operations into one copying the value of the
sqlstate variable pointing to the 'best sqlstate' the function could
determine.
---
--- DBD-Pg-eh-fix-2/dbdimp.c 18 Mar 2011 22:20:38 -0000 1.1.1.5
+++ DBD-Pg-eh-fix-2/dbdimp.c 21 Mar 2011 22:46:41 -0000 1.1.1.5.2.4
@@ -81,6 +81,7 @@ typedef enum
static void pg_error(pTHX_ SV *h, int error_num, const char *error_msg);
static void pg_warn (void * arg, const char * message);
static ExecStatusType _result(pTHX_ imp_dbh_t *imp_dbh, const char *sql);
+static void _fatal_sqlstate(pTHX_ imp_dbh_t *imp_dbh);
static ExecStatusType _sqlstate(pTHX_ imp_dbh_t *imp_dbh, PGresult *result);
static int pg_db_rollback_commit (pTHX_ SV *dbh, imp_dbh_t *imp_dbh, int
action);
static void pg_st_split_statement (pTHX_ imp_sth_t *imp_sth, int version, char
*statement);
@@ -356,13 +357,24 @@ static ExecStatusType _result(pTHX_ imp_
} /* end of _result */
+/* ================================================================== */
+/* Set the SQLSTATE for a 'fatal' error */
+static void _fatal_sqlstate(pTHX_ imp_dbh_t * imp_dbh)
+{
+ char *sqlstate;
+
+ sqlstate = PQstatus(imp_dbh->conn) == CONNECTION_BAD ?
+ "08000" : /* CONNECTION EXCEPTION */
+ "22000"; /* DATA EXCEPTION */
+ strncpy(imp_dbh->sqlstate, sqlstate, 6);
+}
/* ================================================================== */
/* Set the SQLSTATE based on a result, returns the status */
static ExecStatusType _sqlstate(pTHX_ imp_dbh_t * imp_dbh, PGresult * result)
{
+ char *sqlstate;
ExecStatusType status = PGRES_FATAL_ERROR; /* until proven otherwise
*/
- bool stateset = DBDPG_FALSE;
if (TSTART) TRC(DBILOGFP, "%sBegin _sqlstate\n", THEADER);
@@ -371,6 +383,8 @@ static ExecStatusType _sqlstate(pTHX_ im
status = PQresultStatus(result);
}
+ sqlstate = NULL;
+
/*
Because PQresultErrorField may not work completely when an error
occurs, and
we are connecting over TCP/IP, only set it here if non-null, and fall
through
@@ -378,14 +392,10 @@ static ExecStatusType _sqlstate(pTHX_ im
*/
if (result) {
TRACE_PQRESULTERRORFIELD;
- if (NULL != PQresultErrorField(result,PG_DIAG_SQLSTATE)) {
- TRACE_PQRESULTERRORFIELD;
- strncpy(imp_dbh->sqlstate,
PQresultErrorField(result,PG_DIAG_SQLSTATE), 5);
- imp_dbh->sqlstate[5] = '\0';
- stateset = DBDPG_TRUE;
- }
+ sqlstate = PQresultErrorField(result, PG_DIAG_SQLSTATE);
}
- if (!stateset) {
+
+ if (!sqlstate) {
/* Do our best to map the status result to a sqlstate code */
switch (status) {
case PGRES_EMPTY_QUERY:
@@ -393,24 +403,28 @@ static ExecStatusType _sqlstate(pTHX_ im
case PGRES_TUPLES_OK:
case PGRES_COPY_OUT:
case PGRES_COPY_IN:
- strncpy(imp_dbh->sqlstate, "00000", 6); /* SUCCESSFUL
COMPLETION */
+ sqlstate = "00000"; /* SUCCESSFUL
COMPLETION */
break;
case PGRES_BAD_RESPONSE:
case PGRES_NONFATAL_ERROR:
- strncpy(imp_dbh->sqlstate, "01000", 6); /* WARNING */
+ sqlstate = "01000"; /* WARNING */
break;
case PGRES_FATAL_ERROR:
- if (!result) { /* libpq returned null - some sort of
connection problem */
- strncpy(imp_dbh->sqlstate, "08000", 6); /*
CONNECTION EXCEPTION */
+ /* libpq returns NULL result in case of connection
failures */
+ if (!result || PQstatus(imp_dbh->conn) ==
CONNECTION_BAD) {
+ sqlstate = "08000"; /* CONNECTION EXCEPTION
*/
break;
}
/*@fallthrough@*/
default:
- strncpy(imp_dbh->sqlstate, "22000", 6); /* DATA
EXCEPTION */
+ sqlstate = "22000"; /* DATA EXCEPTION */
break;
}
}
+ strncpy(imp_dbh->sqlstate, sqlstate, 5);
+ imp_dbh->sqlstate[5] = 0;
+
if (TEND) TRC(DBILOGFP, "%sEnd _sqlstate (imp_dbh->sqlstate: %s)\n",
THEADER, imp_dbh->sqlstate);
@@ -1356,7 +1370,9 @@ SV * pg_db_pg_notifies (SV * dbh, imp_db
TRACE_PQCONSUMEINPUT;
status = PQconsumeInput(imp_dbh->conn);
- if (0 == status) {
+ if (0 == status) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_pg_notifies (error)\n",
THEADER);
@@ -2745,6 +2761,8 @@ int pg_quickexec (SV * dbh, const char *
TRACE_PQSENDQUERY;
if (! PQsendQuery(imp_dbh->conn, sql)) {
if (TRACE4) TRC(DBILOGFP, "%sPQsendQuery failed\n",
THEADER);
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, status,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_quickexec (error:
async do failed)\n", THEADER);
@@ -3725,6 +3743,8 @@ pg_db_putline (SV * dbh, const char * bu
TRACE_PQPUTCOPYDATA;
copystatus = PQputCopyData(imp_dbh->conn, buffer, (int)strlen(buffer));
if (-1 == copystatus) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_putline (error: copystatus
not -1)\n", THEADER);
@@ -3773,6 +3793,8 @@ pg_db_getline (SV * dbh, SV * svbuf, int
return -1;
}
else if (copystatus < 1) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
}
@@ -3815,6 +3837,8 @@ pg_db_getcopydata (SV * dbh, SV * datali
else if (0 == copystatus) { /* async and still in progress: consume and
return */
TRACE_PQCONSUMEINPUT;
if (!PQconsumeInput(imp_dbh->conn)) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_getcopydata
(error: async in progress)\n", THEADER);
@@ -3837,6 +3861,8 @@ pg_db_getcopydata (SV * dbh, SV * datali
}
}
else {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
}
@@ -3874,6 +3900,8 @@ pg_db_putcopydata (SV * dbh, SV * datali
else if (0 == copystatus) { /* non-blocking mode only */
}
else {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
}
@@ -3937,6 +3965,8 @@ int pg_db_putcopyend (SV * dbh)
return 0;
}
else {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_putcopyend (error:
copystatus unknown)\n", THEADER);
@@ -3964,6 +3994,8 @@ int pg_db_endcopy (SV * dbh)
TRACE_PQPUTCOPYEND;
copystatus = PQputCopyEnd(imp_dbh->conn, NULL);
if (-1 == copystatus) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ dbh, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_endcopy
(error)\n", THEADER);
@@ -4707,6 +4739,8 @@ int pg_db_ready(SV *h, imp_dbh_t *imp_db
TRACE_PQCONSUMEINPUT;
if (!PQconsumeInput(imp_dbh->conn)) {
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
TRACE_PQERRORMESSAGE;
pg_error(aTHX_ h, PGRES_FATAL_ERROR,
PQerrorMessage(imp_dbh->conn));
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_ready (error: consume
failed)\n", THEADER);
@@ -4761,6 +4795,9 @@ int pg_db_cancel(SV *h, imp_dbh_t *imp_d
TRACE_PQFREECANCEL;
PQfreeCancel(cancel);
if (TRACEWARN) { TRC(DBILOGFP, "%sPQcancel failed: %s\n",
THEADER, errbuf); }
+
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
pg_error(aTHX_ h, PGRES_FATAL_ERROR, "PQcancel failed");
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_cancel (error: cancel
failed)\n", THEADER);
return DBDPG_FALSE;
@@ -4776,14 +4813,14 @@ int pg_db_cancel(SV *h, imp_dbh_t *imp_d
/* Read in the result - assume only one */
TRACE_PQGETRESULT;
result = PQgetResult(imp_dbh->conn);
+ status = _sqlstate(aTHX_ imp_dbh, result);
+
if (!result) {
pg_error(aTHX_ h, PGRES_FATAL_ERROR, "Failed to get a result
after PQcancel");
if (TEND) TRC(DBILOGFP, "%sEnd pg_db_cancel (error: no
result)\n", THEADER);
return DBDPG_FALSE;
}
- status = _sqlstate(aTHX_ imp_dbh, result);
-
TRACE_PQCLEAR;
PQclear(result);
@@ -4855,6 +4892,9 @@ static int handle_old_async(pTHX_ SV * h
cresult = PQcancel(cancel,errbuf,255);
if (! cresult) {
if (TRACEWARN) { TRC(DBILOGFP, "%sPQcancel
failed: %s\n", THEADER, errbuf); }
+
+ _fatal_sqlstate(aTHX_ imp_dbh);
+
pg_error(aTHX_ handle, PGRES_FATAL_ERROR,
"Could not cancel previous command");
if (TEND) TRC(DBILOGFP, "%sEnd handle_old_async
(error: could not cancel)\n", THEADER);
return -2;
