> On 29 Apr 2025, at 02:10, Jacob Champion <jacob.champ...@enterprisedb.com> > wrote: > > On Wed, Apr 23, 2025 at 10:46 AM Jacob Champion > <jacob.champ...@enterprisedb.com> wrote: >> Are there any readers who feel like an internal ABI version for >> `struct pg_conn`, bumped during breaking backports, would be >> acceptable? (More definitively: are there any readers who would veto >> that?) > > 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. > This will now handle in-place minor version upgrades that swap pg_conn > internals around, so I've gone back to -MAJOR versioning alone. > fe_oauth_state is still exported; it now has an ABI warning above it. > (I figure that's easier to draw a line around during backports, > compared to everything in PGconn. We can still break things there > during major version upgrades.) While I'm far from the expert on this subject (luckily there are such in this thread), I am unable to see any sharp edges from reading and testing this version of the patch. A few small comments: +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.) We should either clarify that it was never shipped as part of libpq core, or remove this altogether. I would vote for the latter since we typically don't document changes that happen during the devcycle. How about something like: +libpq-oauth is an optional module implementing the Device Authorization flow for +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.) +- void libpq_oauth_init(pgthreadlock_t threadlock, + <snip> +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. I think this explanatory paragraph should come before the function prototype. The following paragraph on the setters/getters make sense where it is though. +#if defined(USE_DYNAMIC_OAUTH) && defined(LIBPQ_INT_H) +#error do not rely on libpq-int.h in libpq-oauth.so +#endif 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"? -- Daniel Gustafsson
