Em seg., 27 de mai. de 2024 às 13:47, Jelte Fennema-Nio <postg...@jeltef.nl> escreveu:
> On Mon, 27 May 2024 at 18:16, Ranier Vilela <ranier...@gmail.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
