Re: libpq async duplicate error results
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Andres Freund <andres@anarazel.de>,
Alvaro Herrera <alvherre@alvh.no-ip.org>,
Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-02-16T23:51:37Z
Lists: pgsql-hackers
Attachments
- dont-return-same-error-multiple-times-1.patch (text/x-diff) patch
I wrote: >>> An idea that I don't have time to pursue right now is to track >>> how much of conn->errorMessage has been read out by PQgetResult, >>> and only report newly-added text in later PQgetResult calls of >>> a series, while PQerrorMessage would continue to report the >>> whole thing. Buffer resets would occur where they do now. > I spent a bit more time poking at this issue. I no longer like the > idea of reporting only the tail part of conn->errorMessage; I'm > afraid that would convert "edge cases sometimes return redundant text" > into "edge cases sometimes miss returning text at all", which is not > an improvement. I figured out what seems like an adequately bulletproof solution to this problem. The thing that was scaring me before is that there are code paths in which we'll discard a pending conn->result PGresult (which could be an error) and make a new one; it wasn't entirely clear that we could track which result represents what text. The solution to that has two components: 1. Never advance the how-much-text-has-been-reported counter until we are just about to return an error PGresult to the application. This reduces the risk of missing or duplicated text to what seems an acceptable level. I found that pqPrepareAsyncResult can be used to handle this logic -- it's only called at outer levels, and it already has some state-updating behavior, so we'd already have bugs if it were called improperly. 2. Don't make error PGresults at all until we reach pqPrepareAsyncResult. This saves some cycles in the paths where we previously replaced one error with another. Importantly, it also means that we can now have a bulletproof solution to the problem of how to return an error when we get OOM on any attempt to make a PGresult. The attached patch includes a statically-declared PGresult that we can return in that case. It's not very practical to do #2 exactly, because in the case of an error received from the server, we need to build a PGresult that includes the broken-down error fields. But all internally-generated errors can be handled in this style. This seems workable, and you'll notice it fixes the duplicated text in the test case Andres was worried about. Note that this patch is on top of the event-proc changes I posted in [1]. Without that change, it's not very clear how to cope with PGEVT_RESULTCREATE failure after we've already updated the error status. regards, tom lane [1] https://www.postgresql.org/message-id/3390587.1645035110%40sss.pgh.pa.us
Commits
-
libpq: drop pending pipelined commands in pqDropConnection().
- 93909599cdba 15.0 landed
-
Adjust interaction of libpq pipeline mode with errorMessage resets.
- b15f254466ae 15.0 landed
-
Rearrange libpq's error reporting to avoid duplicated error text.
- 618c16707a6d 15.0 landed
-
In libpq, always append new error messages to conn->errorMessage.
- ffa2e4670123 14.0 cited