On Wed, Apr 30, 2025 at 5:55 AM Daniel Gustafsson <dan...@yesql.se> wrote:
> > To keep things moving: I assume this is unacceptable. So v10 redirects
> > every access to a PGconn struct member through a shim, similarly to
> > how conn->errorMessage was translated in v9. This adds plenty of new
> > boilerplate, but not a whole lot of complexity. To try to keep us
> > honest, libpq-int.h has been removed from the libpq-oauth includes.
>
> That admittedly seems like a win regardless.

Yeah, it moves us much closer to the long-term goal.

> We should either clarify that it was never shipped as part of libpq core, or
> remove this altogether.

Done in v11, with your suggested wording.

> I think this explanatory paragraph should come before the function prototype.

Done.

> Nitpick, but it won't be .so everywhere.  Would this be clearar if spelled out
> with something like "do not rely on libpq-int.h when building libpq-oauth as
> dynamic shared lib"?

I went with "do not rely on libpq-int.h in dynamic builds of
libpq-oauth", since devs are hopefully going to be the only people who
see it. I've also fixed up an errant #endif label right above it.

I'd ideally like to get a working split in for beta. Barring
objections, I plan to get this pushed tomorrow so that the buildfarm
has time to highlight any corner cases well before the Saturday
freeze. I still see the choice of naming (with its forced-ABI break
every major version) as needing more scrutiny, and probably worth a
Revisit entry.

The CI still looks happy, and I will spend today with VMs and more
testing on the Autoconf side. I'll try to peer at Alpine and musl
libc, too; dogfish and basilisk are the Curl-enabled animals that
caught my attention most.

Thanks!
--Jacob
1:  e86e93f7ac8 ! 1:  5a1d1345919 oauth: Move the builtin flow into a separate 
module
    @@ Commit message
     
         Per request from Tom Lane and Bruce Momjian. Based on an initial patch
         by Daniel Gustafsson, who also contributed docs changes. The "bare"
    -    dlopen() concept came from Thomas Munro. Many many people reviewed the
    -    design and implementation; thank you!
    +    dlopen() concept came from Thomas Munro. Many people reviewed the 
design
    +    and implementation; thank you!
     
         Co-authored-by: Daniel Gustafsson <dan...@yesql.se>
         Reviewed-by: Andres Freund <and...@anarazel.de>
         Reviewed-by: Christoph Berg <m...@debian.org>
    +    Reviewed-by: Daniel Gustafsson <dan...@yesql.se>
         Reviewed-by: Jelte Fennema-Nio <postg...@jeltef.nl>
         Reviewed-by: Peter Eisentraut <pe...@eisentraut.org>
         Reviewed-by: Wolfgang Walther <walt...@technowledgy.de>
    @@ src/interfaces/libpq-oauth/Makefile (new)
      ## src/interfaces/libpq-oauth/README (new) ##
     @@
     +libpq-oauth is an optional module implementing the Device Authorization 
flow for
    -+OAuth clients (RFC 8628). It was originally developed as part of libpq 
core and
    -+later split out as its own shared library in order to isolate its 
dependency on
    -+libcurl. (End users who don't want the Curl dependency can simply choose 
not to
    -+install this module.)
    ++OAuth clients (RFC 8628). It is maintained as its own shared library in 
order to
    ++isolate its dependency on libcurl. (End users who don't want the Curl 
dependency
    ++can simply choose not to install this module.)
     +
     +If a connection string allows the use of OAuth, and the server asks for 
it, and
     +a libpq client has not installed its own custom OAuth flow, libpq will 
attempt
    @@ src/interfaces/libpq-oauth/README (new)
     +pg_fe_run_oauth_flow and pg_fe_cleanup_oauth_flow are implementations of
     +conn->async_auth and conn->cleanup_async_auth, respectively.
     +
    ++At the moment, pg_fe_run_oauth_flow() relies on libpq's pg_g_threadlock 
and
    ++libpq_gettext(), which must be injected by libpq using this initialization
    ++function before the flow is run:
    ++
     +- void libpq_oauth_init(pgthreadlock_t threadlock,
     +                                          libpq_gettext_func gettext_impl,
     +                                          conn_errorMessage_func 
errmsg_impl,
    @@ src/interfaces/libpq-oauth/README (new)
     +                                          set_conn_altsock_func 
setaltsock_impl,
     +                                          set_conn_oauth_token_func 
settoken_impl);
     +
    -+At the moment, pg_fe_run_oauth_flow() relies on libpq's pg_g_threadlock 
and
    -+libpq_gettext(), which must be injected by libpq using this initialization
    -+function before the flow is run.
    -+
     +It also relies on access to several members of the PGconn struct. Not 
only can
     +these change positions across minor versions, but the offsets aren't 
necessarily
     +stable within a single minor release (conn->errorMessage, for instance, 
can
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c => 
src/interfaces/libpq-oauth/oauth-cu
     +#define set_conn_altsock(CONN, VAL) do { CONN->altsock = VAL; } while (0)
     +#define set_conn_oauth_token(CONN, VAL) do { CONN->oauth_token = VAL; } 
while (0)
     +
    -+#endif                                                    /* 
!USE_DYNAMIC_OAUTH */
    ++#endif                                                    /* 
USE_DYNAMIC_OAUTH */
     +
     +/* One final guardrail against accidental inclusion... */
     +#if defined(USE_DYNAMIC_OAUTH) && defined(LIBPQ_INT_H)
    -+#error do not rely on libpq-int.h in libpq-oauth.so
    ++#error do not rely on libpq-int.h in dynamic builds of libpq-oauth
     +#endif
      
      /*
    @@ src/interfaces/libpq-oauth/oauth-utils.c (new)
     +#endif
     +
     +#ifdef LIBPQ_INT_H
    -+#error do not rely on libpq-int.h in libpq-oauth
    ++#error do not rely on libpq-int.h in dynamic builds of libpq-oauth
     +#endif
     +
     +/*

Attachment: v11-0001-oauth-Move-the-builtin-flow-into-a-separate-modu.patch
Description: Binary data

Reply via email to