Re: Fix calloc check if oom (PQcancelCreate)

Em seg., 27 de mai. de 2024 às 13:47, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl>
escreveu:

> On Mon, 27 May 2024 at 18:16, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> > Is it mandatory to call PQcancelFinish in case PQcancelCreate fails?
>
>
> Yes, see the following line in the docs:
>
> Note that when PQcancelCreate returns a non-null pointer, you must
> call PQcancelFinish when you are finished with it, in order to dispose
> of the structure and any associated memory blocks. This must be done
> even if the cancel request failed or was abandoned.
>
> Source:
> https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-PQCANCELCREATE
>
> > IMO, I would expect problems with users.
>
> This pattern is taken from regular connection creation and is really
> common in libpq code so I don't expect issues. And even if there were
> an issue, your v1 patch would not be nearly enough. Because you're
> only freeing the connhost and addr field now in case of OOM. But there
> are many more fields that would need to be freed.
>
> >> And that function will free any
> >> partially initialized PGconn correctly. This is also how
> >> pqConnectOptions2 works.
> >
> > Well, I think that function *pqReleaseConnHost*, is incomplete.
> > 1. Don't free connhost field;
>
> ehm... it does free that afaict? It only doesn't set it to NULL. Which
> indeed would be good to do, but not doing so doesn't cause any issues
> with it's current 2 usages afaict.
>
> > 2. Don't free addr field;
>
> That's what release_conn_addrinfo is for.
>
> > 3. Leave nconnhost and naddr, non-zero.
>
> I agree that it would be good to do that pqReleaseConnHost and
> release_conn_addrinfo. But also here, I don't see any issues caused by
> not doing that currently.
>
> So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
> improved for easier safe usage in the future, but I don't think those
> improvements should be grouped into the same commit with an actual
> bugfix.
>
Thanks for detailed comments.
I agreed to apply the trivial fix.

Would you like to take charge of pqReleaseConnHost and
release_conn_addrinfo improvements?

best regards,
Ranier Vilela