On 19/08/2025 23:49, Jelte Fennema-Nio wrote:
> On Thu, 14 Aug 2025 at 15:10, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Here's a new set of patches, to disconnect on OOM instead of hanging or
>> silently discarding messages:
>
> Code looks good. Som small nitpicks though.
>
> This change seems unnecessary, i.e. free(NULL) is a no-op
>
> - free(svname);
> + if (svname)
> + free(svname);
And even more so, this is unreachable when svname == NULL. Thanks!
> Small wording suggestion, maybe change this:
>
> The caller has already saved the error message in conn->errorMessage.
>
> to
>
> The caller should have already saved the error message in conn->errorMessage.
>
> or even
>
> The caller should have already saved the error message using
> libpq_append_conn_error.
I kept it as is, because we use similar wording in a few other places.
Some places do write the message directly in conn->errorMessage without
using libpq_append_conn_error.
Pushed and backpatched to v18. I feel the OOM handling commit would be
appropriate to backpatch further, but since it's pretty intricate code
and we haven't gotten any complaints from the field, I only backpatched
it to v18 for now. We can backpatch it further later if needed.
Thanks for the original patch and the review!
- Heikki
