On Wed, Feb 25, 2026 at 9:16 AM Zsolt Parragi <[email protected]> wrote:
> How would this work with library clients exactly (and even multiple
> library clients)?
The global setting? Not very well at all. The eventual solution needs
to be per-connection.
I don't think any version of v3-0006 is likely to make it for PG19,
for the record. I just need to ensure that the use case we're moving
towards continues to work with the code that's actually committed, and
there still might be some pieces that should be cherry-picked
backwards in the set. As I said in my original post:
> That brings us to patch [v1-]0007, which experimentally promotes the stable
> API to a public header, and introduces a really janky environment
> variable so that people don't have to play games. It will be obvious
> from the code that this is not well-baked yet.
My hope was to continue to take small steps in the direction of the
end goal with every release, but I think PGOAUTHMODULE is a step too
far. I don't want to expose an oauth_module connection option, and an
environment variable that can't be subsumed by a future connection
option will just be cruft eventually.
For 19, bleeding-edge developers who want a global override ASAP can
implement the public API and tell their application's linker to find a
different libpq-oauth. If a global override isn't good enough,
PGOAUTHMODULE wouldn't have helped anyway.
> * Language bindings: there's already ongoing work on adding support
> for the hooks into these libraries, so that's not really a use case
(The hooks are probably insufficient in the general case [1], but a
global envvar isn't intended to solve that.)
> * postgres CLI tools: That's one use case, but it could work without
> generic user-facing plugin support, limited to these tools.
Yes.
> * postgres_fwd/dblink and similar: I'm not sure how that would work,
> what if different extensions require different plugins? This already
> seems like a tricky question with the hooks
We don't support proxied OAuth at all yet (i.e. both of those
extensions outright prohibit oauth_* settings).
> I can't just download
> a binary application, and a separate oauth plugin, and use them
> together, I need admin permission - that seems strange to me.
Why does that seem strange? If you don't have the ability to install
libpq to begin with, you shouldn't be able to modify that libpq or get
it to run arbitrary code. If you control the application, on the other
hand, you control both the global hook and the link behavior (you
don't have to link against system libpq, after all), so it doesn't
seem like you've lost any functionality.
> An easy solution could be using secure_getenv instead of getenv, which
> would at least improve the situation?
I don't think we claim setuid-safety for libpq. (Our own code aside,
we'd have to vet all of our transitive dependencies in perpetuity.)
> And a few specific comments about the patches:
Thank you for the review!
> +oom:
> + request->error = libpq_gettext("out of memory");
> + return -1;
>
> Shouldn't this also free conninfo if it is allocated?
Yep, good catch.
> +/* Features added in PostgreSQL v19: */
> +/* Indicates presence of PQgetThreadLock */
> +#define LIBPQ_HAS_GET_THREAD_LOCK
>
> Should this be defined to 1?
Yes. One of my weirder copy-paste errors...
> + if ((lockerr = pthread_mutex_lock(&init_mutex)) != 0)
> + {
> + /* Should not happen... but don't continue if it does. */
> + Assert(false);
>
> - libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr);
> - return 0;
> - }
> + libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr);
> + return 0;
> + }
>
> Shouldn't this path return -1,
We could. I chose zero to try to retain the PG18 behavior, but I could
expand this error message and set request->error instead. If that'd be
less confusing to you as a reader, it's probably worth the change.
> and also dlclose the library?
I don't think we gain anything by that, do we? It's named correctly,
it has the right symbols, and the
shouldn't-happen-failure-that-happened had nothing to do with the
library implementation.
> Shouldn't dlcloses also reset state->flow_module after?
That'd probably be kinder to anyone expanding this logic in the
future, yeah. Maybe not worth a backpatch though (there's no users
outside of this function; it's only retained to assist with debugging
at the moment).
> -free_async_ctx(PGconn *conn, struct async_ctx *actx)
> +free_async_ctx(PGoauthBearerRequestV2 *req, struct async_ctx *actx)
>
> req (or conn) doesn't seem to be used in this function, does it need
> that parameter?
Ah, right, it's dead now that req->error isn't set. Thanks!
> + env = strchr(env, '\x01');
> + *env++ = '\0';
>
> Isn't mutating environment variables UB?
Kinda, or at least it invites UB later on according to POSIX. I should
just make a copy.
Thanks,
--Jacob
[1] https://github.com/ged/ruby-pg/pull/693#issuecomment-3867178201
