Re: BackendKeyData is mandatory?

On 08/08/2025 09:44, Jelte Fennema-Nio wrote:
> On Fri, 8 Aug 2025 at 00:03, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.
> 
> You're right. I think I simply forgot to remove that in v3 (it was
> necessary for v2). I'd say let's remove that check and keep the error
> path closer to the behavior in other places.

Ok.

I noticed that there's a similar, existing case in getNotify(), where 
libpq just hangs if an allocation fails. To simulate that, apply this 
change and use LISTEN/NOTIFY:

--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1587,7 +1587,7 @@ getNotify(PGconn *conn)
      if (pqGets(&conn->workBuffer, conn))
          return EOF;
      /* must save name while getting extra string */
-    svname = strdup(conn->workBuffer.data);
+    svname = NULL;
      if (!svname)
          return EOF;
      if (pqGets(&conn->workBuffer, conn))

Returning EOF means "not enough data", which is wrong here just like in 
getBackendKeyData().

I'm not sure how to best fix that. If we can't process a Notify message 
because of out of memory, what should we do?

a) silently drop the Notify messsage.
b) report an error on the next query
c) drop the connection with the error.

You went with c) in getBackendKeyData(), which makes sense since that 
happens when establishing a connection. But Notify messages can be 
received at any time. b) is appealing, but I'm a little worried if we 
can manage the state correctly. Returning an error implies that the 
transaction is aborted, but since this error is generated internally in 
libpq, the transaction is not affected.

I spotted one more case in pqSaveParameterStatus(): if the malloc() 
there fails, it just silently skips storing the parameter, so that a 
later call to PQparameterStatus() will not find it. That also seems bad.

- Heikki