Re: BackendKeyData is mandatory?

(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 <postgres(at)jeltef(dot)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