(reviewing patch 2 now)
On 03/07/2025 09:13, Jelte Fennema-Nio wrote:
On Thu Jul 3, 2025 at 2:03 AM CEST, Jacob Champion wrote:
On Wed, Jul 2, 2025 at 3:18 PM Jelte Fennema-Nio <postg...@jeltef.nl>
wrote:
I will hold off on detailed review until Heikki gives an opinion on
the design (or we get closer to the end of the month), to avoid making
busy work for you -- but I will say that I think you need to prove
that the new `failure:` case in getBackendKeyData() is safe, because I
don't think any of the other failure modes behave that way inside
pqParseInput3().
I changed it slightly now to align with the handleSyncLoss function its
implementation.
That seems good, except maybe the copy-pasted comments could be adjusted
a bit. But I wonder why you added this:
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4322,6 +4322,9 @@ keep_going:
\
/* We will come back to here until there is
if (PQisBusy(conn))
return PGRES_POLLING_READING;
+ if (conn->status == CONNECTION_BAD)
+ goto error_return;
+
res = PQgetResult(conn);
/*
That was not necessary for handleSyncLoss() to work, or for any other
errors. If an error has occurred, PQgetResult() returns an error result,
which is handled here.
It's not necessarily a bad idea. It saves some effort, as PQgetResult()
doesn't need to construct the result object, which we will just
immediately free again. But we have been doing fine without it.
- Heikki
