Improve connection-failure error handling in contrib/postgres_fdw.

postgres_fdw tended to say "unknown error" if it tried to execute a command
on an already-dead connection, because some paths in libpq just return a
null PGresult for such cases.  Out-of-memory might result in that, too.
To fix, pass the PGconn to pgfdw_report_error, and look at its
PQerrorMessage() string if we can't get anything out of the PGresult.

Also, fix the transaction-exit logic to reliably drop a dead connection.
It was attempting to do that already, but it assumed that only connection
cache entries with xact_depth > 0 needed to be examined.  The folly in that
is that if we fail while issuing START TRANSACTION, we'll not have bumped
xact_depth.  (At least for the case I was testing, this fix masks the
other problem; but it still seems like a good idea to have the PGconn
fallback logic.)

Per investigation of bug #9087 from Craig Lucas.  Backpatch to 9.3 where
this code was introduced.

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 93ea498012609c1fdba918f3a29d964b14e0baf5..b688241283805d7d0c57375c90ccd610967def5e 100644 (file)
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -355,7 +355,7 @@ do_sql_command(PGconn *conn, const char *sql)
 
    res = PQexec(conn, sql);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-       pgfdw_report_error(ERROR, res, true, sql);
+       pgfdw_report_error(ERROR, res, conn, true, sql);
    PQclear(res);
 }
 
@@ -454,6 +454,7 @@ GetPrepStmtNumber(PGconn *conn)
  *
  * elevel: error level to use (typically ERROR, but might be less)
  * res: PGresult containing the error
+ * conn: connection we did the query on
  * clear: if true, PQclear the result (otherwise caller will handle it)
  * sql: NULL, or text of remote command we tried to execute
  *
@@ -462,7 +463,8 @@ GetPrepStmtNumber(PGconn *conn)
  * marked with have_error = true.
  */
 void
-pgfdw_report_error(int elevel, PGresult *res, bool clear, const char *sql)
+pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
+                  bool clear, const char *sql)
 {
    /* If requested, PGresult must be released before leaving this function. */
    PG_TRY();
@@ -483,6 +485,14 @@ pgfdw_report_error(int elevel, PGresult *res, bool clear, const char *sql)
        else
            sqlstate = ERRCODE_CONNECTION_FAILURE;
 
+       /*
+        * If we don't get a message from the PGresult, try the PGconn.  This
+        * is needed because for connection-level failures, PQexec may just
+        * return NULL, not a PGresult at all.
+        */
+       if (message_primary == NULL)
+           message_primary = PQerrorMessage(conn);
+
        ereport(elevel,
                (errcode(sqlstate),
                 message_primary ? errmsg_internal("%s", message_primary) :
@@ -525,74 +535,37 @@ pgfdw_xact_callback(XactEvent event, void *arg)
    {
        PGresult   *res;
 
-       /* We only care about connections with open remote transactions */
-       if (entry->conn == NULL || entry->xact_depth == 0)
+       /* Ignore cache entry if no open connection right now */
+       if (entry->conn == NULL)
            continue;
 
-       elog(DEBUG3, "closing remote transaction on connection %p",
-            entry->conn);
-
-       switch (event)
+       /* If it has an open remote transaction, try to close it */
+       if (entry->xact_depth > 0)
        {
-           case XACT_EVENT_PRE_COMMIT:
-               /* Commit all remote transactions during pre-commit */
-               do_sql_command(entry->conn, "COMMIT TRANSACTION");
-
-               /*
-                * If there were any errors in subtransactions, and we made
-                * prepared statements, do a DEALLOCATE ALL to make sure we
-                * get rid of all prepared statements.  This is annoying and
-                * not terribly bulletproof, but it's probably not worth
-                * trying harder.
-                *
-                * DEALLOCATE ALL only exists in 8.3 and later, so this
-                * constrains how old a server postgres_fdw can communicate
-                * with.  We intentionally ignore errors in the DEALLOCATE, so
-                * that we can hobble along to some extent with older servers
-                * (leaking prepared statements as we go; but we don't really
-                * support update operations pre-8.3 anyway).
-                */
-               if (entry->have_prep_stmt && entry->have_error)
-               {
-                   res = PQexec(entry->conn, "DEALLOCATE ALL");
-                   PQclear(res);
-               }
-               entry->have_prep_stmt = false;
-               entry->have_error = false;
-               break;
-           case XACT_EVENT_PRE_PREPARE:
-
-               /*
-                * We disallow remote transactions that modified anything,
-                * since it's not really reasonable to hold them open until
-                * the prepared transaction is committed.  For the moment,
-                * throw error unconditionally; later we might allow read-only
-                * cases.  Note that the error will cause us to come right
-                * back here with event == XACT_EVENT_ABORT, so we'll clean up
-                * the connection state at that point.
-                */
-               ereport(ERROR,
-                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                        errmsg("cannot prepare a transaction that modified remote tables")));
-               break;
-           case XACT_EVENT_COMMIT:
-           case XACT_EVENT_PREPARE:
-               /* Should not get here -- pre-commit should have handled it */
-               elog(ERROR, "missed cleaning up connection during pre-commit");
-               break;
-           case XACT_EVENT_ABORT:
-               /* Assume we might have lost track of prepared statements */
-               entry->have_error = true;
-               /* If we're aborting, abort all remote transactions too */
-               res = PQexec(entry->conn, "ABORT TRANSACTION");
-               /* Note: can't throw ERROR, it would be infinite loop */
-               if (PQresultStatus(res) != PGRES_COMMAND_OK)
-                   pgfdw_report_error(WARNING, res, true,
-                                      "ABORT TRANSACTION");
-               else
-               {
-                   PQclear(res);
-                   /* As above, make sure we've cleared any prepared stmts */
+           elog(DEBUG3, "closing remote transaction on connection %p",
+                entry->conn);
+
+           switch (event)
+           {
+               case XACT_EVENT_PRE_COMMIT:
+                   /* Commit all remote transactions during pre-commit */
+                   do_sql_command(entry->conn, "COMMIT TRANSACTION");
+
+                   /*
+                    * If there were any errors in subtransactions, and we
+                    * made prepared statements, do a DEALLOCATE ALL to make
+                    * sure we get rid of all prepared statements. This is
+                    * annoying and not terribly bulletproof, but it's
+                    * probably not worth trying harder.
+                    *
+                    * DEALLOCATE ALL only exists in 8.3 and later, so this
+                    * constrains how old a server postgres_fdw can
+                    * communicate with.  We intentionally ignore errors in
+                    * the DEALLOCATE, so that we can hobble along to some
+                    * extent with older servers (leaking prepared statements
+                    * as we go; but we don't really support update operations
+                    * pre-8.3 anyway).
+                    */
                    if (entry->have_prep_stmt && entry->have_error)
                    {
                        res = PQexec(entry->conn, "DEALLOCATE ALL");
@@ -600,8 +573,50 @@ pgfdw_xact_callback(XactEvent event, void *arg)
                    }
                    entry->have_prep_stmt = false;
                    entry->have_error = false;
-               }
-               break;
+                   break;
+               case XACT_EVENT_PRE_PREPARE:
+
+                   /*
+                    * We disallow remote transactions that modified anything,
+                    * since it's not very reasonable to hold them open until
+                    * the prepared transaction is committed.  For the moment,
+                    * throw error unconditionally; later we might allow
+                    * read-only cases.  Note that the error will cause us to
+                    * come right back here with event == XACT_EVENT_ABORT, so
+                    * we'll clean up the connection state at that point.
+                    */
+                   ereport(ERROR,
+                           (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                            errmsg("cannot prepare a transaction that modified remote tables")));
+                   break;
+               case XACT_EVENT_COMMIT:
+               case XACT_EVENT_PREPARE:
+                   /* Pre-commit should have closed the open transaction */
+                   elog(ERROR, "missed cleaning up connection during pre-commit");
+                   break;
+               case XACT_EVENT_ABORT:
+                   /* Assume we might have lost track of prepared statements */
+                   entry->have_error = true;
+                   /* If we're aborting, abort all remote transactions too */
+                   res = PQexec(entry->conn, "ABORT TRANSACTION");
+                   /* Note: can't throw ERROR, it would be infinite loop */
+                   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+                       pgfdw_report_error(WARNING, res, entry->conn, true,
+                                          "ABORT TRANSACTION");
+                   else
+                   {
+                       PQclear(res);
+                       /* As above, make sure to clear any prepared stmts */
+                       if (entry->have_prep_stmt && entry->have_error)
+                       {
+                           res = PQexec(entry->conn, "DEALLOCATE ALL");
+                           PQclear(res);
+                       }
+                       entry->have_prep_stmt = false;
+                       entry->have_error = false;
+                   }
+                   break;
+           }
        }
 
        /* Reset state to show we're out of a transaction */
@@ -689,7 +704,7 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
                     curlevel, curlevel);
            res = PQexec(entry->conn, sql);
            if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(WARNING, res, true, sql);
+               pgfdw_report_error(WARNING, res, entry->conn, true, sql);
            else
                PQclear(res);
        }

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index ae3ab000e1f34a4a665676d869713b7f396d5dcf..fde1ec13617d80ead676860a57a07b7c648575a4 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1040,7 +1040,7 @@ postgresReScanForeignScan(ForeignScanState *node)
     */
    res = PQexec(fsstate->conn, sql);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-       pgfdw_report_error(ERROR, res, true, sql);
+       pgfdw_report_error(ERROR, res, fsstate->conn, true, sql);
    PQclear(res);
 
    /* Now force a fresh FETCH. */
@@ -1374,7 +1374,7 @@ postgresExecForeignInsert(EState *estate,
                         0);
    if (PQresultStatus(res) !=
        (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-       pgfdw_report_error(ERROR, res, true, fmstate->query);
+       pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
 
    /* Check number of rows affected, and fetch RETURNING tuple if any */
    if (fmstate->has_returning)
@@ -1444,7 +1444,7 @@ postgresExecForeignUpdate(EState *estate,
                         0);
    if (PQresultStatus(res) !=
        (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-       pgfdw_report_error(ERROR, res, true, fmstate->query);
+       pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
 
    /* Check number of rows affected, and fetch RETURNING tuple if any */
    if (fmstate->has_returning)
@@ -1514,7 +1514,7 @@ postgresExecForeignDelete(EState *estate,
                         0);
    if (PQresultStatus(res) !=
        (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-       pgfdw_report_error(ERROR, res, true, fmstate->query);
+       pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
 
    /* Check number of rows affected, and fetch RETURNING tuple if any */
    if (fmstate->has_returning)
@@ -1563,7 +1563,7 @@ postgresEndForeignModify(EState *estate,
         */
        res = PQexec(fmstate->conn, sql);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-           pgfdw_report_error(ERROR, res, true, sql);
+           pgfdw_report_error(ERROR, res, fmstate->conn, true, sql);
        PQclear(res);
        fmstate->p_name = NULL;
    }
@@ -1800,7 +1800,7 @@ get_remote_estimate(const char *sql, PGconn *conn,
         */
        res = PQexec(conn, sql);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-           pgfdw_report_error(ERROR, res, false, sql);
+           pgfdw_report_error(ERROR, res, conn, false, sql);
 
        /*
         * Extract cost numbers for topmost plan node.  Note we search for a
@@ -1934,7 +1934,7 @@ create_cursor(ForeignScanState *node)
    res = PQexecParams(conn, buf.data, numParams, NULL, values,
                       NULL, NULL, 0);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-       pgfdw_report_error(ERROR, res, true, fsstate->query);
+       pgfdw_report_error(ERROR, res, conn, true, fsstate->query);
    PQclear(res);
 
    /* Mark the cursor as created, and show no tuples have been retrieved */
@@ -1985,7 +1985,7 @@ fetch_more_data(ForeignScanState *node)
        res = PQexec(conn, sql);
        /* On error, report the original query, not the FETCH. */
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-           pgfdw_report_error(ERROR, res, false, fsstate->query);
+           pgfdw_report_error(ERROR, res, conn, false, fsstate->query);
 
        /* Convert the data into HeapTuples */
        numrows = PQntuples(res);
@@ -2091,7 +2091,7 @@ close_cursor(PGconn *conn, unsigned int cursor_number)
     */
    res = PQexec(conn, sql);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-       pgfdw_report_error(ERROR, res, true, sql);
+       pgfdw_report_error(ERROR, res, conn, true, sql);
    PQclear(res);
 }
 
@@ -2128,7 +2128,7 @@ prepare_foreign_modify(PgFdwModifyState *fmstate)
                    NULL);
 
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-       pgfdw_report_error(ERROR, res, true, fmstate->query);
+       pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
    PQclear(res);
 
    /* This action shows that the prepare has been done. */
@@ -2278,7 +2278,7 @@ postgresAnalyzeForeignTable(Relation relation,
    {
        res = PQexec(conn, sql.data);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-           pgfdw_report_error(ERROR, res, false, sql.data);
+           pgfdw_report_error(ERROR, res, conn, false, sql.data);
 
        if (PQntuples(res) != 1 || PQnfields(res) != 1)
            elog(ERROR, "unexpected result from deparseAnalyzeSizeSql query");
@@ -2372,7 +2372,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
    {
        res = PQexec(conn, sql.data);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-           pgfdw_report_error(ERROR, res, false, sql.data);
+           pgfdw_report_error(ERROR, res, conn, false, sql.data);
        PQclear(res);
        res = NULL;
 
@@ -2403,7 +2403,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
            res = PQexec(conn, fetch_sql);
            /* On error, report the original query, not the FETCH. */
            if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, false, sql.data);
+               pgfdw_report_error(ERROR, res, conn, false, sql.data);
 
            /* Process whatever we got. */
            numrows = PQntuples(res);

diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 85fc25aaef4a1bcd0050bd1e679efb1abc2d46ee..228345d78644df7972d95e5d4129e4920d6e6874 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -30,8 +30,8 @@ extern PGconn *GetConnection(ForeignServer *server, UserMapping *user,
 extern void ReleaseConnection(PGconn *conn);
 extern unsigned int GetCursorNumber(PGconn *conn);
 extern unsigned int GetPrepStmtNumber(PGconn *conn);
-extern void pgfdw_report_error(int elevel, PGresult *res, bool clear,
-                  const char *sql);
+extern void pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
+                  bool clear, const char *sql);
 
 /* in option.c */
 extern int ExtractConnectionOptions(List *defelems,