Thread

1. BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   PG Bug reporting form <noreply@postgresql.org> — 2025-06-18T19:25:04Z

   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.
   
   

2. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-18T21:09:03Z

   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
   
   
   
   

3. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-21T20:01:03Z

   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
   
   

4. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Dmitry Kovalenko <d.kovalenko@postgrespro.ru> — 2025-06-21T20:42:42Z

   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
   
   
   
   
   

5. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-21T21:46:18Z

   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
   
   
   
   

6. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-03T12:28:30Z

   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)
   
   
   
   

7. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-03T14:48:17Z

   =?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
   
   
   
   

8. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-03T17:06:02Z

   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
   
   
   
   

9. Re: BUG #18960: Mistake in test test_simple_pipeline (libpq_pipeline.c)

   Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-03T20:10:52Z

   =?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