Re: BackendKeyData is mandatory? - Mailing list pgsql-hackers

(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@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