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.
