Skip Menu |

This queue is for tickets about the DBD-Pg CPAN distribution.

Report information
The Basics
Id: 66792
Status: resolved
Priority: 0/
Queue: DBD-Pg

People
Owner: greg [...] turnstep.com
Requestors: rweikusat [...] mssgmbh.com
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: (no value)
Fixed in: 3.0.0



Subject: Consistently report CONNECTION_BAD state to applications.
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.
Subject: patch.txt
--- 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;
Thanks, we'll look this over; probably once the upcoming 2.18.0 is out the door.
Subject: Re: [rt.cpan.org #66792] Consistently report CONNECTION_BAD state to applications.
Date: Sun, 27 Mar 2011 19:37:22 +0000
To: bug-DBD-Pg [...] rt.cpan.org
From: Rainer Weikusat <rweikusat [...] mobileactivedefense.com>
"Greg Sabino Mullane via RT" <bug-DBD-Pg@rt.cpan.org> writes: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=66792 > > > Thanks, we'll look this over; probably once the upcoming 2.18.0 is out > the door.
JFTR: The patch is actually based on the 2.18.0 SVN tree.
Applied in 202ae11f0723f05a6b43f7a49bd70300274162e1