Split up pgfdw_report_error so that we can mark it pg_noreturn.

pgfdw_report_error has the same design fault as elog/ereport
do, namely that it might or might not return depending on elevel.
While those functions are too widely used to redesign, there are
only about 30 call sites for pgfdw_report_error, and it's not
exposed for extension use.  So let's rethink it.  Split it into
pgfdw_report_error() which hard-wires ERROR elevel and is marked
pg_noreturn, and pgfdw_report() which allows only elevels less
than ERROR.  (Thanks to Álvaro Herrera for suggesting this naming.)

The motivation for doing this now is that in the wake of commit
80aa9848b, which removed a bunch of PG_TRYs from postgres_fdw,
we're seeing more thorough flow analysis there from C compilers
and Coverity.  Marking pgfdw_report_error as noreturn where
appropriate should help prevent false-positive complaints.

We could alternatively have invented a macro wrapper similar
to what we use for elog/ereport, but that code is sufficiently
fragile that I didn't find it appetizing to make another copy.
Since 80aa9848b already changed pgfdw_report_error's signature,
this won't make back-patching any harder than it was already.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/420221.1753714491@sss.pgh.pa.us

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index a33843fcf8531b0aec5704cd7c781942d0faea21..e8148f2c5a223f7e9379f74102ea8d59091d4d1b 100644 (file)
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -142,6 +142,8 @@ static void do_sql_command_begin(PGconn *conn, const char *sql);
 static void do_sql_command_end(PGconn *conn, const char *sql,
                                                           bool consume_input);
 static void begin_remote_xact(ConnCacheEntry *entry);
+static void pgfdw_report_internal(int elevel, PGresult *res, PGconn *conn,
+                                                                 const char *sql);
 static void pgfdw_xact_callback(XactEvent event, void *arg);
 static void pgfdw_subxact_callback(SubXactEvent event,
                                                                   SubTransactionId mySubid,
@@ -815,7 +817,7 @@ static void
 do_sql_command_begin(PGconn *conn, const char *sql)
 {
        if (!PQsendQuery(conn, sql))
-               pgfdw_report_error(ERROR, NULL, conn, sql);
+               pgfdw_report_error(NULL, conn, sql);
 }
 
 static void
@@ -830,10 +832,10 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input)
         * would be large compared to the overhead of PQconsumeInput.)
         */
        if (consume_input && !PQconsumeInput(conn))
-               pgfdw_report_error(ERROR, NULL, conn, sql);
+               pgfdw_report_error(NULL, conn, sql);
        res = pgfdw_get_result(conn);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, conn, sql);
+               pgfdw_report_error(res, conn, sql);
        PQclear(res);
 }
 
@@ -966,7 +968,10 @@ pgfdw_get_result(PGconn *conn)
 /*
  * Report an error we got from the remote server.
  *
- * elevel: error level to use (typically ERROR, but might be less)
+ * Callers should use pgfdw_report_error() to throw an error, or use
+ * pgfdw_report() for lesser message levels.  (We make this distinction
+ * so that pgfdw_report_error() can be marked noreturn.)
+ *
  * res: PGresult containing the error (might be NULL)
  * conn: connection we did the query on
  * sql: NULL, or text of remote command we tried to execute
@@ -979,8 +984,22 @@ pgfdw_get_result(PGconn *conn)
  * marked with have_error = true.
  */
 void
-pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
-                                  const char *sql)
+pgfdw_report_error(PGresult *res, PGconn *conn, const char *sql)
+{
+       pgfdw_report_internal(ERROR, res, conn, sql);
+       pg_unreachable();
+}
+
+void
+pgfdw_report(int elevel, PGresult *res, PGconn *conn, const char *sql)
+{
+       Assert(elevel < ERROR);         /* use pgfdw_report_error for that */
+       pgfdw_report_internal(elevel, res, conn, sql);
+}
+
+static void
+pgfdw_report_internal(int elevel, PGresult *res, PGconn *conn,
+                                         const char *sql)
 {
        char       *diag_sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
        char       *message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY);
@@ -1538,7 +1557,7 @@ pgfdw_exec_cleanup_query_begin(PGconn *conn, const char *query)
         */
        if (!PQsendQuery(conn, query))
        {
-               pgfdw_report_error(WARNING, NULL, conn, query);
+               pgfdw_report(WARNING, NULL, conn, query);
                return false;
        }
 
@@ -1563,7 +1582,7 @@ pgfdw_exec_cleanup_query_end(PGconn *conn, const char *query,
         */
        if (consume_input && !PQconsumeInput(conn))
        {
-               pgfdw_report_error(WARNING, NULL, conn, query);
+               pgfdw_report(WARNING, NULL, conn, query);
                return false;
        }
 
@@ -1575,7 +1594,7 @@ pgfdw_exec_cleanup_query_end(PGconn *conn, const char *query,
                                        (errmsg("could not get query result due to timeout"),
                                         errcontext("remote SQL command: %s", query)));
                else
-                       pgfdw_report_error(WARNING, NULL, conn, query);
+                       pgfdw_report(WARNING, NULL, conn, query);
 
                return false;
        }
@@ -1583,7 +1602,7 @@ pgfdw_exec_cleanup_query_end(PGconn *conn, const char *query,
        /* Issue a warning if not successful. */
        if (PQresultStatus(result) != PGRES_COMMAND_OK)
        {
-               pgfdw_report_error(WARNING, result, conn, query);
+               pgfdw_report(WARNING, result, conn, query);
                return ignore_errors;
        }
        PQclear(result);

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 9d266b3e2b1205aae507427782c79f200fa84ea1..456b267f70b5b5f4b8109a89ea8c05318ab22d17 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1704,7 +1704,7 @@ postgresReScanForeignScan(ForeignScanState *node)
 
        res = pgfdw_exec_query(fsstate->conn, sql, fsstate->conn_state);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, fsstate->conn, sql);
+               pgfdw_report_error(res, fsstate->conn, sql);
        PQclear(res);
 
        /* Now force a fresh FETCH. */
@@ -3614,7 +3614,7 @@ get_remote_estimate(const char *sql, PGconn *conn,
         */
        res = pgfdw_exec_query(conn, sql, NULL);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, conn, sql);
+               pgfdw_report_error(res, conn, sql);
 
        /*
         * Extract cost numbers for topmost plan node.  Note we search for a left
@@ -3769,14 +3769,14 @@ create_cursor(ForeignScanState *node)
         */
        if (!PQsendQueryParams(conn, buf.data, numParams,
                                                   NULL, values, NULL, NULL, 0))
-               pgfdw_report_error(ERROR, NULL, conn, buf.data);
+               pgfdw_report_error(NULL, conn, buf.data);
 
        /*
         * Get the result, and check for success.
         */
        res = pgfdw_get_result(conn);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, conn, fsstate->query);
+               pgfdw_report_error(res, conn, fsstate->query);
        PQclear(res);
 
        /* Mark the cursor as created, and show no tuples have been retrieved */
@@ -3823,7 +3823,7 @@ fetch_more_data(ForeignScanState *node)
                res = pgfdw_get_result(conn);
                /* On error, report the original query, not the FETCH. */
                if (PQresultStatus(res) != PGRES_TUPLES_OK)
-                       pgfdw_report_error(ERROR, res, conn, fsstate->query);
+                       pgfdw_report_error(res, conn, fsstate->query);
 
                /* Reset per-connection state */
                fsstate->conn_state->pendingAreq = NULL;
@@ -3839,7 +3839,7 @@ fetch_more_data(ForeignScanState *node)
                res = pgfdw_exec_query(conn, sql, fsstate->conn_state);
                /* On error, report the original query, not the FETCH. */
                if (PQresultStatus(res) != PGRES_TUPLES_OK)
-                       pgfdw_report_error(ERROR, res, conn, fsstate->query);
+                       pgfdw_report_error(res, conn, fsstate->query);
        }
 
        /* Convert the data into HeapTuples */
@@ -3944,7 +3944,7 @@ close_cursor(PGconn *conn, unsigned int cursor_number,
        snprintf(sql, sizeof(sql), "CLOSE c%u", cursor_number);
        res = pgfdw_exec_query(conn, sql, conn_state);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, conn, sql);
+               pgfdw_report_error(res, conn, sql);
        PQclear(res);
 }
 
@@ -4152,7 +4152,7 @@ execute_foreign_modify(EState *estate,
                                                         NULL,
                                                         NULL,
                                                         0))
-               pgfdw_report_error(ERROR, NULL, fmstate->conn, fmstate->query);
+               pgfdw_report_error(NULL, fmstate->conn, fmstate->query);
 
        /*
         * Get the result, and check for success.
@@ -4160,7 +4160,7 @@ execute_foreign_modify(EState *estate,
        res = pgfdw_get_result(fmstate->conn);
        if (PQresultStatus(res) !=
                (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-               pgfdw_report_error(ERROR, res, fmstate->conn, fmstate->query);
+               pgfdw_report_error(res, fmstate->conn, fmstate->query);
 
        /* Check number of rows affected, and fetch RETURNING tuple if any */
        if (fmstate->has_returning)
@@ -4219,14 +4219,14 @@ prepare_foreign_modify(PgFdwModifyState *fmstate)
                                           fmstate->query,
                                           0,
                                           NULL))
-               pgfdw_report_error(ERROR, NULL, fmstate->conn, fmstate->query);
+               pgfdw_report_error(NULL, fmstate->conn, fmstate->query);
 
        /*
         * Get the result, and check for success.
         */
        res = pgfdw_get_result(fmstate->conn);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, fmstate->conn, fmstate->query);
+               pgfdw_report_error(res, fmstate->conn, fmstate->query);
        PQclear(res);
 
        /* This action shows that the prepare has been done. */
@@ -4373,7 +4373,7 @@ deallocate_query(PgFdwModifyState *fmstate)
        snprintf(sql, sizeof(sql), "DEALLOCATE %s", fmstate->p_name);
        res = pgfdw_exec_query(fmstate->conn, sql, fmstate->conn_state);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, fmstate->conn, sql);
+               pgfdw_report_error(res, fmstate->conn, sql);
        PQclear(res);
        pfree(fmstate->p_name);
        fmstate->p_name = NULL;
@@ -4541,7 +4541,7 @@ execute_dml_stmt(ForeignScanState *node)
         */
        if (!PQsendQueryParams(dmstate->conn, dmstate->query, numParams,
                                                   NULL, values, NULL, NULL, 0))
-               pgfdw_report_error(ERROR, NULL, dmstate->conn, dmstate->query);
+               pgfdw_report_error(NULL, dmstate->conn, dmstate->query);
 
        /*
         * Get the result, and check for success.
@@ -4549,7 +4549,7 @@ execute_dml_stmt(ForeignScanState *node)
        dmstate->result = pgfdw_get_result(dmstate->conn);
        if (PQresultStatus(dmstate->result) !=
                (dmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-               pgfdw_report_error(ERROR, dmstate->result, dmstate->conn,
+               pgfdw_report_error(dmstate->result, dmstate->conn,
                                                   dmstate->query);
 
        /*
@@ -4923,7 +4923,7 @@ postgresAnalyzeForeignTable(Relation relation,
 
        res = pgfdw_exec_query(conn, sql.data, NULL);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, conn, sql.data);
+               pgfdw_report_error(res, conn, sql.data);
 
        if (PQntuples(res) != 1 || PQnfields(res) != 1)
                elog(ERROR, "unexpected result from deparseAnalyzeSizeSql query");
@@ -4972,7 +4972,7 @@ postgresGetAnalyzeInfoForForeignTable(Relation relation, bool *can_tablesample)
 
        res = pgfdw_exec_query(conn, sql.data, NULL);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, conn, sql.data);
+               pgfdw_report_error(res, conn, sql.data);
 
        if (PQntuples(res) != 1 || PQnfields(res) != 2)
                elog(ERROR, "unexpected result from deparseAnalyzeInfoSql query");
@@ -5202,7 +5202,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 
        res = pgfdw_exec_query(conn, sql.data, NULL);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
-               pgfdw_report_error(ERROR, res, conn, sql.data);
+               pgfdw_report_error(res, conn, sql.data);
        PQclear(res);
 
        /*
@@ -5254,7 +5254,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
                res = pgfdw_exec_query(conn, fetch_sql, NULL);
                /* On error, report the original query, not the FETCH. */
                if (PQresultStatus(res) != PGRES_TUPLES_OK)
-                       pgfdw_report_error(ERROR, res, conn, sql.data);
+                       pgfdw_report_error(res, conn, sql.data);
 
                /* Process whatever we got. */
                numrows = PQntuples(res);
@@ -5426,7 +5426,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
 
        res = pgfdw_exec_query(conn, buf.data, NULL);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, conn, buf.data);
+               pgfdw_report_error(res, conn, buf.data);
 
        if (PQntuples(res) != 1)
                ereport(ERROR,
@@ -5540,7 +5540,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
        /* Fetch the data */
        res = pgfdw_exec_query(conn, buf.data, NULL);
        if (PQresultStatus(res) != PGRES_TUPLES_OK)
-               pgfdw_report_error(ERROR, res, conn, buf.data);
+               pgfdw_report_error(res, conn, buf.data);
 
        /* Process results */
        numrows = PQntuples(res);
@@ -7312,7 +7312,7 @@ postgresForeignAsyncNotify(AsyncRequest *areq)
 
        /* On error, report the original query, not the FETCH. */
        if (!PQconsumeInput(fsstate->conn))
-               pgfdw_report_error(ERROR, NULL, fsstate->conn, fsstate->query);
+               pgfdw_report_error(NULL, fsstate->conn, fsstate->query);
 
        fetch_more_data(node);
 
@@ -7411,7 +7411,7 @@ fetch_more_data_begin(AsyncRequest *areq)
                         fsstate->fetch_size, fsstate->cursor_number);
 
        if (!PQsendQuery(fsstate->conn, sql))
-               pgfdw_report_error(ERROR, NULL, fsstate->conn, fsstate->query);
+               pgfdw_report_error(NULL, fsstate->conn, fsstate->query);
 
        /* Remember that the request is in process */
        fsstate->conn_state->pendingAreq = areq;

diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 38e1a8859413133bf93af3fa3b9a7d2c4f68cc6b..e69735298d78fb54ad14563abc8f4ce83b5de745 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -166,8 +166,10 @@ extern void do_sql_command(PGconn *conn, const char *sql);
 extern PGresult *pgfdw_get_result(PGconn *conn);
 extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query,
                                                                  PgFdwConnState *state);
-extern void pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
-                                                          const char *sql);
+pg_noreturn extern void pgfdw_report_error(PGresult *res, PGconn *conn,
+                                                                                  const char *sql);
+extern void pgfdw_report(int elevel, PGresult *res, PGconn *conn,
+                                                const char *sql);
 
 /* in option.c */
 extern int     ExtractConnectionOptions(List *defelems,