From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: libpq async duplicate error results
Date: Sun, 20 Feb 2022 15:26:56 -0500
Message-ID: <394797.1645388816@sss.pgh.pa.us> (raw)
In-Reply-To: <202202081500.xl3iwa7doscz@alvherre.pgsql>
References: <202202081500.xl3iwa7doscz@alvherre.pgsql>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Feb-07, Tom Lane wrote:
>> I see that PQgetResult does a hacky "resetPQExpBuffer(&conn->errorMessage)"
>> in between pipelined queries. It seems like if there are real usability
>> issues in this area, they probably come from that not being well placed.
>> In particular, I wonder why that's done with the pqPipelineProcessQueue
>> call in the PGASYNC_IDLE stanza, yet not with the pqPipelineProcessQueue
>> call in the PGASYNC_READY stanza (where we're returning a PIPELINE_SYNC
>> result). It kinda looks to me like maybe pqPipelineProcessQueue
>> ought to have the responsibility for doing that, rather than having
>> two out of the three call sites do it. Also it would seem more natural
>> to associate it with the reinitialization that happens inside
>> pqPipelineProcessQueue.
> Yeah, I agree that the placement of error message reset in pipelined
> libpq is more ad-hoc to the testing I was doing than carefully
> principled. I didn't test changing it, but on a quick look your
> proposed change seems reasonable to me.
Here's a patch to do that. It passes check-world (particularly
src/test/modules/libpq_pipeline), for whatever that's worth.
However ... in the wake of 618c16707, I wonder if we should consider
an alternative definition, which is to NOT clear errorMessage when
starting a new pipelined query. (That would be this patch minus
the addition to pqPipelineProcessQueue.) Thanks to 618c16707,
the returned error PGresult(s) should bear exactly the same contents
either way. What would change is that PQerrorMessage(conn) would return
the concatenation of all errors that have occurred during the current
pipeline sequence. I think that's arguably a more useful definition
than what we have now --- though obviously it'd require a docs change.
It seems to fit with the spirit of the v14 changes to ensure that
PQerrorMessage tells you about everything that happened during a
failed connection attempt, not only the last thing.
I've tested that variant, and it also passes check-world, which may
say something about libpq_pipeline's coverage of error cases ...
I didn't look closely.
regards, tom lane
Attachments:centralize-pipeline-errorMessage-reset.patch (text/x-diff, 1.2K) downloaddiff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.cindex 45dddaf556..495d2b4c13 100644--- a/src/interfaces/libpq/fe-exec.c+++ b/src/interfaces/libpq/fe-exec.c@@ -1380,10 +1380,7 @@ pqAppendCmdQueueEntry(PGconn *conn, PGcmdQueueEntry *entry)
* state, we don't have to do anything.
*/
if (conn->asyncStatus == PGASYNC_IDLE)
- {- pqClearConnErrorState(conn);
pqPipelineProcessQueue(conn);
- }
break;
}
}
@@ -2149,11 +2146,8 @@ PQgetResult(PGconn *conn)
/*
* We're about to return the NULL that terminates the round of
* results from the current query; prepare to send the results
- * of the next query when we're called next. Also, since this- * is the start of the results of the next query, clear any- * prior error message.+ * of the next query when we're called next.
*/
- pqClearConnErrorState(conn);
pqPipelineProcessQueue(conn);
}
break;
@@ -3099,6 +3093,9 @@ pqPipelineProcessQueue(PGconn *conn)
conn->cmd_queue_head == NULL)
return;
+ /* Reset the error state */+ pqClearConnErrorState(conn);+
/* Initialize async result-accumulation state */
pqClearAsyncResult(conn);
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@alvh.no-ip.org, coelho@cri.ensmp.fr, peter.eisentraut@enterprisedb.com
Subject: Re: libpq async duplicate error results
In-Reply-To: <394797.1645388816@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox