Обсуждение: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

The following bug has been logged on the website:

Bug reference:      18960
Logged by:          Dmitry Kovalenko
Email address:      d.kovalenko@postgrespro.ru
PostgreSQL version: 18beta1
Operating system:   any
Description:

Hello,
Please look at these test lines in
src/test/modules/libpq_pipeline/libpq_pipeline.c 1657-1662:

https://github.com/postgres/postgres/blob/c2e2589ab969eb802493191c79de844bf7dc3a6e/src/test/modules/libpq_pipeline/libpq_pipeline.c#L1657-L1662
---
        PQclear(res);
        res = NULL;
        if (PQgetResult(conn) != NULL)
                pg_fatal("PQgetResult returned something extra after
pipeline end: %s",
                                 PQresStatus(PQresultStatus(res)));
---
You forgot to assign res:
---
        PQclear(res);
        res = NULL;
        if ((res = PQgetResult(conn)) != NULL)
                pg_fatal("PQgetResult returned something extra after
pipeline end: %s",
                                 PQresStatus(PQresultStatus(res)));
---
Thanks&Regards,
Dmitry Kovalenko
PostgresPro, Russia.

PG Bug reporting form <noreply@postgresql.org> writes:
> Please look at these test lines in
> src/test/modules/libpq_pipeline/libpq_pipeline.c 1657-1662:
>
https://github.com/postgres/postgres/blob/c2e2589ab969eb802493191c79de844bf7dc3a6e/src/test/modules/libpq_pipeline/libpq_pipeline.c#L1657-L1662
> ---
>         PQclear(res);
>         res = NULL;
>         if (PQgetResult(conn) != NULL)
>                 pg_fatal("PQgetResult returned something extra after
> pipeline end: %s",
>                                  PQresStatus(PQresultStatus(res)));
> ---
> You forgot to assign res:
> ---
>         PQclear(res);
>         res = NULL;
>         if ((res = PQgetResult(conn)) != NULL)
>                 pg_fatal("PQgetResult returned something extra after
> pipeline end: %s",
>                                  PQresStatus(PQresultStatus(res)));

I agree that's wrong ... but looking around, there's a huge amount
of random inconsistency in this test script --- this same simple
task of checking for an expected NULL result is coded several
different ways with varying amounts of detail provided, and
some other places share this same outright bug.

I think it'd be better to make a helper function
"CheckNoMoreResults(conn)", or something along that line,
to shorten and standardize these places.

On the other side of the coin, the explicit tests for a result
*not* being NULL are mostly unnecessary; if the next step is
a check of PQresultStatus, we could just rely on the fact
that PQresultStatus(NULL) returns PGRES_FATAL_ERROR.

            regards, tom lane

I wrote:
> I agree that's wrong ... but looking around, there's a huge amount
> of random inconsistency in this test script --- this same simple
> task of checking for an expected NULL result is coded several
> different ways with varying amounts of detail provided, and
> some other places share this same outright bug.

> I think it'd be better to make a helper function
> "CheckNoMoreResults(conn)", or something along that line,
> to shorten and standardize these places.

Here's a shot at improving matters.  I also made an effort
at cleaning up memory leaks in libpq_pipeline.c, although
that's surely neatnik-ism not anything meaningful.

            regards, tom lane

diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 9a3c0236325..ebfdb4a8da3 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -87,6 +87,57 @@ pg_fatal_impl(int line, const char *fmt,...)
     exit(1);
 }

+/*
+ * Check that libpq next returns a PGresult with the specified status,
+ * returning the PGresult so that caller can perform additional checks.
+ */
+#define confirm_result_status_keep(conn, status) confirm_result_status_keep_impl(__LINE__, conn, status)
+static PGresult *
+confirm_result_status_keep_impl(int line, PGconn *conn, ExecStatusType status)
+{
+    PGresult   *res;
+
+    res = PQgetResult(conn);
+    if (res == NULL)
+        pg_fatal_impl(line, "PQgetResult returned null unexpectedly: %s",
+                      PQerrorMessage(conn));
+    if (PQresultStatus(res) != status)
+        pg_fatal_impl(line, "PQgetResult returned status %s, expected %s: %s",
+                      PQresStatus(PQresultStatus(res)),
+                      PQresStatus(status),
+                      PQerrorMessage(conn));
+    return res;
+}
+
+/*
+ * Check that libpq next returns a PGresult with the specified status.
+ */
+#define confirm_result_status(conn, status) confirm_result_status_impl(__LINE__, conn, status)
+static void
+confirm_result_status_impl(int line, PGconn *conn, ExecStatusType status)
+{
+    PGresult   *res;
+
+    res = confirm_result_status_keep_impl(line, conn, status);
+    PQclear(res);
+}
+
+/*
+ * Check that libpq next returns a null PGresult.
+ */
+#define confirm_null_result(conn) confirm_null_result_impl(__LINE__, conn)
+static void
+confirm_null_result_impl(int line, PGconn *conn)
+{
+    PGresult   *res;
+
+    res = PQgetResult(conn);
+    if (res != NULL)
+        pg_fatal_impl(line, "expected NULL PGresult, got %s: %s",
+                      PQresStatus(PQresultStatus(res)),
+                      PQerrorMessage(conn));
+}
+
 /*
  * Check that the query on the given connection got canceled.
  */
@@ -94,14 +145,9 @@ pg_fatal_impl(int line, const char *fmt,...)
 static void
 confirm_query_canceled_impl(int line, PGconn *conn)
 {
-    PGresult   *res = NULL;
+    PGresult   *res;

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal_impl(line, "PQgetResult returned null: %s",
-                      PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_FATAL_ERROR)
-        pg_fatal_impl(line, "query did not fail when it was expected");
+    res = confirm_result_status_keep_impl(line, conn, PGRES_FATAL_ERROR);
     if (strcmp(PQresultErrorField(res, PG_DIAG_SQLSTATE), "57014") != 0)
         pg_fatal_impl(line, "query failed with a different error than cancellation: %s",
                       PQerrorMessage(conn));
@@ -234,6 +280,10 @@ copy_connection(PGconn *conn)
         pg_fatal("Connection to database failed: %s",
                  PQerrorMessage(copyConn));

+    pfree(keywords);
+    pfree(vals);
+    PQconninfoFree(opts);
+
     return copyConn;
 }

@@ -400,6 +450,7 @@ test_cancel(PGconn *conn)
     confirm_query_canceled(conn);

     PQcancelFinish(cancelConn);
+    PQfinish(monitorConn);

     fprintf(stderr, "ok\n");
 }
@@ -428,6 +479,7 @@ test_disallowed_in_pipeline(PGconn *conn)
                "synchronous command execution functions are not allowed in pipeline mode\n") != 0)
         pg_fatal("did not get expected error message; got: \"%s\"",
                  PQerrorMessage(conn));
+    PQclear(res);

     /* PQsendQuery should fail in pipeline mode */
     if (PQsendQuery(conn, "SELECT 1") != 0)
@@ -460,6 +512,7 @@ test_disallowed_in_pipeline(PGconn *conn)
     if (PQresultStatus(res) != PGRES_TUPLES_OK)
         pg_fatal("PQexec should succeed after exiting pipeline mode but failed with: %s",
                  PQerrorMessage(conn));
+    PQclear(res);

     fprintf(stderr, "ok\n");
 }
@@ -467,7 +520,6 @@ test_disallowed_in_pipeline(PGconn *conn)
 static void
 test_multi_pipelines(PGconn *conn)
 {
-    PGresult   *res = NULL;
     const char *dummy_params[1] = {"1"};
     Oid            dummy_param_oids[1] = {INT4OID};

@@ -508,87 +560,31 @@ test_multi_pipelines(PGconn *conn)
     /* OK, start processing the results */

     /* first pipeline */
+    confirm_result_status(conn, PGRES_TUPLES_OK);

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Unexpected result code %s from first pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = NULL;
-
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("PQgetResult returned something extra after first result");
+    confirm_null_result(conn);

     if (PQexitPipelineMode(conn) != 0)
         pg_fatal("exiting pipeline mode after query but before sync succeeded incorrectly");

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when sync result expected: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s instead of sync result, error: %s",
-                 PQresStatus(PQresultStatus(res)), PQerrorMessage(conn));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     /* second pipeline */
+    confirm_result_status(conn, PGRES_TUPLES_OK);

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Unexpected result code %s from second pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = NULL;
-
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("PQgetResult returned something extra after first result");
+    confirm_null_result(conn);

     if (PQexitPipelineMode(conn) != 0)
         pg_fatal("exiting pipeline mode after query but before sync succeeded incorrectly");

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when sync result expected: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s instead of sync result, error: %s",
-                 PQresStatus(PQresultStatus(res)), PQerrorMessage(conn));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     /* third pipeline */
+    confirm_result_status(conn, PGRES_TUPLES_OK);

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
+    confirm_null_result(conn);

-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Unexpected result code %s from third pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("Expected null result, got %s",
-                 PQresStatus(PQresultStatus(res)));
-
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s from second pipeline sync",
-                 PQresStatus(PQresultStatus(res)));
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     /* We're still in pipeline mode ... */
     if (PQpipelineStatus(conn) == PQ_PIPELINE_OFF)
@@ -657,36 +653,17 @@ test_nosync(PGconn *conn)
     /* Now read all results */
     for (;;)
     {
-        PGresult   *res;
-
-        res = PQgetResult(conn);
-
-        /* NULL results are only expected after TUPLES_OK */
-        if (res == NULL)
-            pg_fatal("got unexpected NULL result after %d results", results);
-
         /* We expect exactly one TUPLES_OK result for each query we sent */
-        if (PQresultStatus(res) == PGRES_TUPLES_OK)
-        {
-            PGresult   *res2;
+        confirm_result_status(conn, PGRES_TUPLES_OK);

-            /* and one NULL result should follow each */
-            res2 = PQgetResult(conn);
-            if (res2 != NULL)
-                pg_fatal("expected NULL, got %s",
-                         PQresStatus(PQresultStatus(res2)));
-            PQclear(res);
-            results++;
-
-            /* if we're done, we're done */
-            if (results == numqueries)
-                break;
+        /* and one NULL result should follow each */
+        confirm_null_result(conn);

-            continue;
-        }
+        results++;

-        /* anything else is unexpected */
-        pg_fatal("got unexpected %s\n", PQresStatus(PQresultStatus(res)));
+        /* if we're done, we're done */
+        if (results == numqueries)
+            break;
     }

     fprintf(stderr, "ok\n");
@@ -716,10 +693,12 @@ test_pipeline_abort(PGconn *conn)
     res = PQexec(conn, drop_table_sql);
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("dispatching DROP TABLE failed: %s", PQerrorMessage(conn));
+    PQclear(res);

     res = PQexec(conn, create_table_sql);
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("dispatching CREATE TABLE failed: %s", PQerrorMessage(conn));
+    PQclear(res);

     /*
      * Queue up a couple of small pipelines and process each without returning
@@ -763,33 +742,16 @@ test_pipeline_abort(PGconn *conn)
      * a pipeline aborted message for the second insert, a pipeline-end, then
      * a command-ok and a pipeline-ok for the second pipeline operation.
      */
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("Unexpected result status %s: %s",
-                 PQresStatus(PQresultStatus(res)),
-                 PQresultErrorMessage(res));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_COMMAND_OK);

     /* NULL result to signal end-of-results for this command */
-    if ((res = PQgetResult(conn)) != NULL)
-        pg_fatal("Expected null result, got %s",
-                 PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);

     /* Second query caused error, so we expect an error next */
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_FATAL_ERROR)
-        pg_fatal("Unexpected result code -- expected PGRES_FATAL_ERROR, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_FATAL_ERROR);

     /* NULL result to signal end-of-results for this command */
-    if ((res = PQgetResult(conn)) != NULL)
-        pg_fatal("Expected null result, got %s",
-                 PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);

     /*
      * pipeline should now be aborted.
@@ -802,17 +764,10 @@ test_pipeline_abort(PGconn *conn)
         pg_fatal("pipeline should be flagged as aborted but isn't");

     /* third query in pipeline, the second insert */
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_PIPELINE_ABORTED)
-        pg_fatal("Unexpected result code -- expected PGRES_PIPELINE_ABORTED, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_ABORTED);

     /* NULL result to signal end-of-results for this command */
-    if ((res = PQgetResult(conn)) != NULL)
-        pg_fatal("Expected null result, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);

     if (PQpipelineStatus(conn) != PQ_PIPELINE_ABORTED)
         pg_fatal("pipeline should be flagged as aborted but isn't");
@@ -827,14 +782,7 @@ test_pipeline_abort(PGconn *conn)
      * (This is so clients know to start processing results normally again and
      * can tell the difference between skipped commands and the sync.)
      */
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code from first pipeline sync\n"
-                 "Expected PGRES_PIPELINE_SYNC, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     if (PQpipelineStatus(conn) == PQ_PIPELINE_ABORTED)
         pg_fatal("sync should've cleared the aborted flag but didn't");
@@ -844,30 +792,16 @@ test_pipeline_abort(PGconn *conn)
         pg_fatal("Fell out of pipeline mode somehow");

     /* the insert from the second pipeline */
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("Unexpected result code %s from first item in second pipeline",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_COMMAND_OK);

     /* Read the NULL result at the end of the command */
-    if ((res = PQgetResult(conn)) != NULL)
-        pg_fatal("Expected null result, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);

     /* the second pipeline sync */
-    if ((res = PQgetResult(conn)) == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s from second pipeline sync",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

-    if ((res = PQgetResult(conn)) != NULL)
-        pg_fatal("Expected null result, got %s: %s",
-                 PQresStatus(PQresultStatus(res)),
-                 PQerrorMessage(conn));
+    /* Read the NULL result at the end of the command */
+    confirm_null_result(conn);

     /* Try to send two queries in one command */
     if (PQsendQueryParams(conn, "SELECT 1; SELECT 2", 0, NULL, NULL, NULL, NULL, 0) != 1)
@@ -890,15 +824,14 @@ test_pipeline_abort(PGconn *conn)
                 pg_fatal("got unexpected status %s", PQresStatus(PQresultStatus(res)));
                 break;
         }
+        PQclear(res);
     }
     if (!goterror)
         pg_fatal("did not get cannot-insert-multiple-commands error");
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("got NULL result");
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s from pipeline sync",
-                 PQresStatus(PQresultStatus(res)));
+
+    /* the second pipeline sync */
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);
+
     fprintf(stderr, "ok\n");

     /* Test single-row mode with an error partways */
@@ -935,13 +868,9 @@ test_pipeline_abort(PGconn *conn)
         pg_fatal("did not get division-by-zero error");
     if (gotrows != 3)
         pg_fatal("did not get three rows");
+
     /* the third pipeline sync */
-    if ((res = PQgetResult(conn)) == NULL)
-        pg_fatal("Unexpected NULL result: %s", PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s from third pipeline sync",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     /* We're still in pipeline mode... */
     if (PQpipelineStatus(conn) == PQ_PIPELINE_OFF)
@@ -1274,21 +1203,11 @@ test_prepared(PGconn *conn)
     if (PQpipelineSync(conn) != 1)
         pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null");
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("expected NULL result");
+    confirm_result_status(conn, PGRES_COMMAND_OK);

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned NULL");
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);
+
+    res = confirm_result_status_keep(conn, PGRES_COMMAND_OK);
     if (PQnfields(res) != lengthof(expected_oids))
         pg_fatal("expected %zu columns, got %d",
                  lengthof(expected_oids), PQnfields(res));
@@ -1300,13 +1219,10 @@ test_prepared(PGconn *conn)
                      i, expected_oids[i], typ);
     }
     PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("expected NULL result");

-    res = PQgetResult(conn);
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("expected PGRES_PIPELINE_SYNC, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);
+
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     fprintf(stderr, "closing statement..");
     if (PQsendClosePrepared(conn, "select_one") != 1)
@@ -1314,18 +1230,11 @@ test_prepared(PGconn *conn)
     if (PQpipelineSync(conn) != 1)
         pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("expected non-NULL result");
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("expected NULL result");
-    res = PQgetResult(conn);
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("expected PGRES_PIPELINE_SYNC, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_result_status(conn, PGRES_COMMAND_OK);
+
+    confirm_null_result(conn);
+
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("could not exit pipeline mode: %s", PQerrorMessage(conn));
@@ -1334,6 +1243,7 @@ test_prepared(PGconn *conn)
     res = PQdescribePrepared(conn, "select_one");
     if (PQresultStatus(res) != PGRES_FATAL_ERROR)
         pg_fatal("expected FATAL_ERROR, got %s", PQresStatus(PQresultStatus(res)));
+    PQclear(res);

     /*
      * Also test the blocking close, this should not fail since closing a
@@ -1342,32 +1252,36 @@ test_prepared(PGconn *conn)
     res = PQclosePrepared(conn, "select_one");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
+    PQclear(res);

     fprintf(stderr, "creating portal... ");
-    PQexec(conn, "BEGIN");
-    PQexec(conn, "DECLARE cursor_one CURSOR FOR SELECT 1");
+
+    res = PQexec(conn, "BEGIN");
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pg_fatal("BEGIN failed: %s", PQerrorMessage(conn));
+    PQclear(res);
+
+    res = PQexec(conn, "DECLARE cursor_one CURSOR FOR SELECT 1");
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pg_fatal("DECLARE CURSOR failed: %s", PQerrorMessage(conn));
+    PQclear(res);
+
     PQenterPipelineMode(conn);
     if (PQsendDescribePortal(conn, "cursor_one") != 1)
         pg_fatal("PQsendDescribePortal failed: %s", PQerrorMessage(conn));
     if (PQpipelineSync(conn) != 1)
         pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null");
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));

+    res = confirm_result_status_keep(conn, PGRES_COMMAND_OK);
     typ = PQftype(res, 0);
     if (typ != INT4OID)
         pg_fatal("portal: expected type %u, got %u",
                  INT4OID, typ);
     PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("expected NULL result");
-    res = PQgetResult(conn);
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("expected PGRES_PIPELINE_SYNC, got %s", PQresStatus(PQresultStatus(res)));
+
+    confirm_null_result(conn);
+
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     fprintf(stderr, "closing portal... ");
     if (PQsendClosePortal(conn, "cursor_one") != 1)
@@ -1375,18 +1289,11 @@ test_prepared(PGconn *conn)
     if (PQpipelineSync(conn) != 1)
         pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("expected non-NULL result");
-    if (PQresultStatus(res) != PGRES_COMMAND_OK)
-        pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("expected NULL result");
-    res = PQgetResult(conn);
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("expected PGRES_PIPELINE_SYNC, got %s", PQresStatus(PQresultStatus(res)));
+    confirm_result_status(conn, PGRES_COMMAND_OK);
+
+    confirm_null_result(conn);
+
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("could not exit pipeline mode: %s", PQerrorMessage(conn));
@@ -1395,6 +1302,7 @@ test_prepared(PGconn *conn)
     res = PQdescribePortal(conn, "cursor_one");
     if (PQresultStatus(res) != PGRES_FATAL_ERROR)
         pg_fatal("expected FATAL_ERROR, got %s", PQresStatus(PQresultStatus(res)));
+    PQclear(res);

     /*
      * Also test the blocking close, this should not fail since closing a
@@ -1403,6 +1311,7 @@ test_prepared(PGconn *conn)
     res = PQclosePortal(conn, "cursor_one");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
+    PQclear(res);

     fprintf(stderr, "ok\n");
 }
@@ -1509,6 +1418,10 @@ test_protocol_version(PGconn *conn)
         pg_fatal("expected 30002, got %d", protocol_version);

     PQfinish(conn);
+
+    pfree(keywords);
+    pfree(vals);
+    PQconninfoFree(opts);
 }

 /* Notice processor: print notices, and count how many we got */
@@ -1525,7 +1438,6 @@ notice_processor(void *arg, const char *message)
 static void
 test_pipeline_idle(PGconn *conn)
 {
-    PGresult   *res;
     int            n_notices = 0;

     fprintf(stderr, "\npipeline idle...\n");
@@ -1538,17 +1450,11 @@ test_pipeline_idle(PGconn *conn)
     if (PQsendQueryParams(conn, "SELECT 1", 0, NULL, NULL, NULL, NULL, 0) != 1)
         pg_fatal("failed to send query: %s", PQerrorMessage(conn));
     PQsendFlushRequest(conn);
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("unexpected result code %s from first pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("did not receive terminating NULL");
+
+    confirm_result_status(conn, PGRES_TUPLES_OK);
+
+    confirm_null_result(conn);
+
     if (PQsendQueryParams(conn, "SELECT 2", 0, NULL, NULL, NULL, NULL, 0) != 1)
         pg_fatal("failed to send query: %s", PQerrorMessage(conn));
     if (PQexitPipelineMode(conn) == 1)
@@ -1558,14 +1464,11 @@ test_pipeline_idle(PGconn *conn)
         pg_fatal("did not get expected error; got: %s",
                  PQerrorMessage(conn));
     PQsendFlushRequest(conn);
-    res = PQgetResult(conn);
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("unexpected result code %s from second pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-    PQclear(res);
-    res = PQgetResult(conn);
-    if (res != NULL)
-        pg_fatal("did not receive terminating NULL");
+
+    confirm_result_status(conn, PGRES_TUPLES_OK);
+
+    confirm_null_result(conn);
+
     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("exiting pipeline failed: %s", PQerrorMessage(conn));

@@ -1579,11 +1482,9 @@ test_pipeline_idle(PGconn *conn)
     if (PQsendQueryParams(conn, "SELECT pg_catalog.pg_advisory_unlock(1,1)", 0, NULL, NULL, NULL, NULL, 0) != 1)
         pg_fatal("failed to send query: %s", PQerrorMessage(conn));
     PQsendFlushRequest(conn);
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL result received");
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("unexpected result code %s", PQresStatus(PQresultStatus(res)));
+
+    confirm_result_status(conn, PGRES_TUPLES_OK);
+
     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("failed to exit pipeline mode: %s", PQerrorMessage(conn));
     fprintf(stderr, "ok - 2\n");
@@ -1592,7 +1493,6 @@ test_pipeline_idle(PGconn *conn)
 static void
 test_simple_pipeline(PGconn *conn)
 {
-    PGresult   *res = NULL;
     const char *dummy_params[1] = {"1"};
     Oid            dummy_param_oids[1] = {INT4OID};

@@ -1623,20 +1523,9 @@ test_simple_pipeline(PGconn *conn)
     if (PQpipelineSync(conn) != 1)
         pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Unexpected result code %s from first pipeline item",
-                 PQresStatus(PQresultStatus(res)));
-
-    PQclear(res);
-    res = NULL;
+    confirm_result_status(conn, PGRES_TUPLES_OK);

-    if (PQgetResult(conn) != NULL)
-        pg_fatal("PQgetResult returned something extra after first query result.");
+    confirm_null_result(conn);

     /*
      * Even though we've processed the result there's still a sync to come and
@@ -1645,21 +1534,9 @@ test_simple_pipeline(PGconn *conn)
     if (PQexitPipelineMode(conn) != 0)
         pg_fatal("exiting pipeline mode after query but before sync succeeded incorrectly");

-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("PQgetResult returned null when sync result PGRES_PIPELINE_SYNC expected: %s",
-                 PQerrorMessage(conn));
-
-    if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
-        pg_fatal("Unexpected result code %s instead of PGRES_PIPELINE_SYNC, error: %s",
-                 PQresStatus(PQresultStatus(res)), PQerrorMessage(conn));
-
-    PQclear(res);
-    res = NULL;
+    confirm_result_status(conn, PGRES_PIPELINE_SYNC);

-    if (PQgetResult(conn) != NULL)
-        pg_fatal("PQgetResult returned something extra after pipeline end: %s",
-                 PQresStatus(PQresultStatus(res)));
+    confirm_null_result(conn);

     /* We're still in pipeline mode... */
     if (PQpipelineStatus(conn) == PQ_PIPELINE_OFF)
@@ -1792,20 +1669,12 @@ test_singlerowmode(PGconn *conn)
         pg_fatal("failed to send flush request");
     if (PQsetSingleRowMode(conn) != 1)
         pg_fatal("PQsetSingleRowMode() failed");
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_SINGLE_TUPLE)
-        pg_fatal("Expected PGRES_SINGLE_TUPLE, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Expected PGRES_TUPLES_OK, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("expected NULL result");
+
+    confirm_result_status(conn, PGRES_SINGLE_TUPLE);
+
+    confirm_result_status(conn, PGRES_TUPLES_OK);
+
+    confirm_null_result(conn);

     if (PQsendQueryParams(conn, "SELECT 1",
                           0, NULL, NULL, NULL, NULL, 0) != 1)
@@ -1813,14 +1682,10 @@ test_singlerowmode(PGconn *conn)
                  PQerrorMessage(conn));
     if (PQsendFlushRequest(conn) != 1)
         pg_fatal("failed to send flush request");
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Expected PGRES_TUPLES_OK, got %s",
-                 PQresStatus(PQresultStatus(res)));
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("expected NULL result");
+
+    confirm_result_status(conn, PGRES_TUPLES_OK);
+
+    confirm_null_result(conn);

     /*
      * Try chunked mode as well; make sure that it correctly delivers a
@@ -1834,33 +1699,23 @@ test_singlerowmode(PGconn *conn)
         pg_fatal("failed to send flush request");
     if (PQsetChunkedRowsMode(conn, 3) != 1)
         pg_fatal("PQsetChunkedRowsMode() failed");
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_TUPLES_CHUNK)
-        pg_fatal("Expected PGRES_TUPLES_CHUNK, got %s: %s",
-                 PQresStatus(PQresultStatus(res)),
-                 PQerrorMessage(conn));
+
+    res = confirm_result_status_keep(conn, PGRES_TUPLES_CHUNK);
     if (PQntuples(res) != 3)
         pg_fatal("Expected 3 rows, got %d", PQntuples(res));
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_TUPLES_CHUNK)
-        pg_fatal("Expected PGRES_TUPLES_CHUNK, got %s",
-                 PQresStatus(PQresultStatus(res)));
+    PQclear(res);
+
+    res = confirm_result_status_keep(conn, PGRES_TUPLES_CHUNK);
     if (PQntuples(res) != 2)
         pg_fatal("Expected 2 rows, got %d", PQntuples(res));
-    res = PQgetResult(conn);
-    if (res == NULL)
-        pg_fatal("unexpected NULL");
-    if (PQresultStatus(res) != PGRES_TUPLES_OK)
-        pg_fatal("Expected PGRES_TUPLES_OK, got %s",
-                 PQresStatus(PQresultStatus(res)));
+    PQclear(res);
+
+    res = confirm_result_status_keep(conn, PGRES_TUPLES_OK);
     if (PQntuples(res) != 0)
         pg_fatal("Expected 0 rows, got %d", PQntuples(res));
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("expected NULL result");
+    PQclear(res);
+
+    confirm_null_result(conn);

     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("failed to end pipeline mode: %s", PQerrorMessage(conn));
@@ -1995,9 +1850,8 @@ test_transaction(PGconn *conn)
         if (num_syncs <= 0)
             break;
     }
-    if (PQgetResult(conn) != NULL)
-        pg_fatal("returned something extra after all the syncs: %s",
-                 PQresStatus(PQresultStatus(res)));
+
+    confirm_null_result(conn);

     if (PQexitPipelineMode(conn) != 1)
         pg_fatal("failed to end pipeline mode: %s", PQerrorMessage(conn));
@@ -2053,16 +1907,19 @@ test_uniqviol(PGconn *conn)
                  "create table ppln_uniqviol(id bigint primary key, idata bigint)");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("failed to create table: %s", PQerrorMessage(conn));
+    PQclear(res);

     res = PQexec(conn, "begin");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("failed to begin transaction: %s", PQerrorMessage(conn));
+    PQclear(res);

     res = PQprepare(conn, "insertion",
                     "insert into ppln_uniqviol values ($1, $2) returning id",
                     2, paramTypes);
-    if (res == NULL || PQresultStatus(res) != PGRES_COMMAND_OK)
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("failed to prepare query: %s", PQerrorMessage(conn));
+    PQclear(res);

     if (PQenterPipelineMode(conn) != 1)
         pg_fatal("failed to enter pipeline mode");
@@ -2191,7 +2048,6 @@ test_uniqviol(PGconn *conn)
 static bool
 process_result(PGconn *conn, PGresult *res, int results, int numsent)
 {
-    PGresult   *res2;
     bool        got_error = false;

     if (res == NULL)
@@ -2203,29 +2059,19 @@ process_result(PGconn *conn, PGresult *res, int results, int numsent)
             got_error = true;
             fprintf(stderr, "result %d/%d (error): %s\n", results, numsent, PQerrorMessage(conn));
             PQclear(res);
-
-            res2 = PQgetResult(conn);
-            if (res2 != NULL)
-                pg_fatal("expected NULL, got %s",
-                         PQresStatus(PQresultStatus(res2)));
+            confirm_null_result(conn);
             break;

         case PGRES_TUPLES_OK:
             fprintf(stderr, "result %d/%d: %s\n", results, numsent, PQgetvalue(res, 0, 0));
             PQclear(res);
-
-            res2 = PQgetResult(conn);
-            if (res2 != NULL)
-                pg_fatal("expected NULL, got %s",
-                         PQresStatus(PQresultStatus(res2)));
+            confirm_null_result(conn);
             break;

         case PGRES_PIPELINE_ABORTED:
             fprintf(stderr, "result %d/%d: pipeline aborted\n", results, numsent);
-            res2 = PQgetResult(conn);
-            if (res2 != NULL)
-                pg_fatal("expected NULL, got %s",
-                         PQresStatus(PQresultStatus(res2)));
+            PQclear(res);
+            confirm_null_result(conn);
             break;

         default:
@@ -2271,7 +2117,7 @@ main(int argc, char **argv)
 {
     const char *conninfo = "";
     PGconn       *conn;
-    FILE       *trace;
+    FILE       *trace = NULL;
     char       *testname;
     int            numrows = 10000;
     PGresult   *res;
@@ -2332,9 +2178,11 @@ main(int argc, char **argv)
     res = PQexec(conn, "SET lc_messages TO \"C\"");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("failed to set \"lc_messages\": %s", PQerrorMessage(conn));
+    PQclear(res);
     res = PQexec(conn, "SET debug_parallel_query = off");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
         pg_fatal("failed to set \"debug_parallel_query\": %s", PQerrorMessage(conn));
+    PQclear(res);

     /* Set the trace file, if requested */
     if (tracefile != NULL)
@@ -2388,5 +2236,9 @@ main(int argc, char **argv)

     /* close the connection to the database and cleanup */
     PQfinish(conn);
+
+    if (trace && trace != stdout)
+        fclose(trace);
+
     return 0;
 }

Hello Tom,

Thank you for your interest in this problem.

+    res = PQgetResult(conn);
+    if (res == NULL)
+        pg_fatal_impl(line, "PQgetResult returned null unexpectedly: %s",
+                      PQerrorMessage(conn));
+    if (PQresultStatus(res) != status)
+        pg_fatal_impl(line, "PQgetResult returned status %s, expected 
%s: %s",
+                      PQresStatus(PQresultStatus(res)),
+                      PQresStatus(status),
+                      PQerrorMessage(conn));
+    return res;

As I understand, you have leaks of 'res' when (PQresultStatus(res) != 
status)

I spent the some time to fix a leaks in tests and PG itself here:

https://github.com/dmitry-lipetsk/postgres/commits/D20250617_001-pg_master/

libpq_pipeline.c has not been finished yet but "make check" (under ASAN) 
can already executed without any problems.

I hope, you will find something useful in this branch.

For example, these problems are obvious and can be trivially fixed:

https://github.com/dmitry-lipetsk/postgres/commit/484ec68fcfea77beadc19387721d04ca77abe55f

https://github.com/dmitry-lipetsk/postgres/commit/3213f95cfd3e094d6ece4a9114f30f0204c0d056

https://github.com/dmitry-lipetsk/postgres/commit/0d4497a744bd00e741434aa6e307ed23f348fbc4

https://github.com/dmitry-lipetsk/postgres/commit/6f966657d7e9409dcbd8109d41bdc89f22024324 


I am planning to return and finish this work near time - I want to get a 
clean execution of "make check-world" under ASAN.

Thanks&Regards,

Dmitry Kovalenko

Dmitry Kovalenko <d.kovalenko@postgrespro.ru> writes:
> As I understand, you have leaks of 'res' when (PQresultStatus(res) != 
> status)

I don't think we really care about leaks when pg_fatal is called.
(If we do, there are a lot of others to worry about.)

> I spent the some time to fix a leaks in tests and PG itself here:
> https://github.com/dmitry-lipetsk/postgres/commits/D20250617_001-pg_master/

Ah, I did not know you were working on this.  Perhaps you can merge
what we've done.

            regards, tom lane

On 2025-Jun-21, Tom Lane wrote:

> Here's a shot at improving matters.  I also made an effort
> at cleaning up memory leaks in libpq_pipeline.c, although
> that's surely neatnik-ism not anything meaningful.

Yeah, the code looks much better this way.  I thought it was a bit odd
that a function called confirm_result_status() would actually consume
said status.  Would it be better as
  consume_result_status(PGconn *conn, ExecStatusType expected)
?

I think the patch is a clear improvement regardless.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-Jun-21, Tom Lane wrote:
>> Here's a shot at improving matters.  I also made an effort
>> at cleaning up memory leaks in libpq_pipeline.c, although
>> that's surely neatnik-ism not anything meaningful.

> Yeah, the code looks much better this way.  I thought it was a bit odd
> that a function called confirm_result_status() would actually consume
> said status.  Would it be better as
>   consume_result_status(PGconn *conn, ExecStatusType expected)
> ?

Hm, I chose that name by analogy to the adjacent
confirm_query_canceled(), which is likewise consuming a result.
I agree that "consume" is a better verb, but then let's rename
confirm_query_canceled as well.

> I think the patch is a clear improvement regardless.

Thanks for reviewing!

            regards, tom lane

On 2025-Sep-03, Tom Lane wrote:

> Hm, I chose that name by analogy to the adjacent
> confirm_query_canceled(), which is likewise consuming a result.
> I agree that "consume" is a better verb, but then let's rename
> confirm_query_canceled as well.

Ah, that explains it -- my bad, then, I suppose.  That change works for
me.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)
https://postgr.es/m/20050809113420.GD2768@phlogiston.dyndns.org

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-Sep-03, Tom Lane wrote:
>> Hm, I chose that name by analogy to the adjacent
>> confirm_query_canceled(), which is likewise consuming a result.
>> I agree that "consume" is a better verb, but then let's rename
>> confirm_query_canceled as well.

> Ah, that explains it -- my bad, then, I suppose.  That change works for
> me.

After reflection, I used "consume_xxx" for the void-returning helper
functions, but kept the "confirm_xxx" terminology for the helper
function that returns a PGresult after confirming its status is
as-expected.  Pushed with those renamings.

            regards, tom lane