Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

On Mon, Jan 20, 2025 at 2:10 PM Daniel Gustafsson <daniel@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