On Mon, Jan 20, 2025 at 2:10 PM Daniel Gustafsson <dan...@yesql.se> wrote:
> + /*
> + * The mechanism should have set up the necessary callbacks; all we
> + * need to do is signal the caller.
> + */
> + *async = true;
> + return STATUS_OK;
> Is it worth adding assertions here to ensure that everything has been set up
> properly to help when adding a new mechanism in the future?
Yeah, I think that'd be helpful.
> + /* Done. Tear down the async implementation. */
> + conn->cleanup_async_auth(conn);
> + conn->cleanup_async_auth = NULL;
> + Assert(conn->altsock == PGINVALID_SOCKET);
> In pqDropConnection() we set ->altsock to NULL
(I assume you mean PGINVALID_SOCKET?)
> just to be sure rather than
> assert that cleanup has done so. Shouldn't we be consistent in the
> expectation and set to NULL here as well?
I'm not opposed; I just figured that the following code might be a bit
confusing:
Assert(conn->altsock == PGINVALID_SOCKET);
conn->altsock = PGINVALID_SOCKET;
But I can add a comment to the assignment to try to explain. I don't
know what the likelihood of landing code that trips that assertion is,
but an explicit assignment would at least stop problems from
cascading.
--Jacob
