Re: BackendKeyData is mandatory?

On Fri, 8 Aug 2025 at 10:42, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> 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.

Nice finds. I think c) would be a very defensible position even for
these two. Sure these two messages might not be critical to always
receive correctly for all clients, but for some they might. And even
for the ones where it's not critical, if malloc fails for these
allocations, it's likely a next malloc for some other purpose will
also fail. Relieving memory pressure by cleaning the connection seems
a pretty sensible thing to do in such a case.

On Fri, 8 Aug 2025 at 10:42, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 08/08/2025 09:44, Jelte Fennema-Nio wrote:
> > On Fri, 8 Aug 2025 at 00:03, Heikki Linnakangas <hlinnaka(at)iki(dot)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
>