Hi, On 2025-04-07 09:41:25 -0700, Jacob Champion wrote: > On Mon, Apr 7, 2025 at 7:21 AM Andres Freund <and...@anarazel.de> wrote: > > On 2025-04-04 17:27:46 -0700, Jacob Champion wrote: > > I think otherwise > > we run into the danger of the wrong library version being loaded in some > > cases. Imagine a program being told with libpq to use via rpath. But then we > > load the oauth module via a dlopen without a specified path - it'll just > > search the global library locations. > > Ah, you mean if the RPATH'd build doesn't have a flow, but the > globally installed one (with a different ABI) does? Yeah, that would > be a problem.
That and more: Even if the RPATH libpq does have oauth support, the libpq-oauth won't necessarily be at the front of the global library search path. So afaict you'll often get a different libpq-oauth. Except perhaps on macos, where all this stuff works differently again. But I managed to unload the required knowledge out of my brain and don't want to further think about that :) > > > +# src/interfaces/libpq-oauth/exports.txt > > > +pg_fe_run_oauth_flow 1 > > > +pg_fe_cleanup_oauth_flow 2 > > > +pg_g_threadlock 3 > > > > The pg_g_threadlock thing seems pretty ugly. Can't we just pass that to the > > relevant functions? > > We can do it however we want, honestly, especially since the ABI isn't > public/stable. I chose this way just to ease review. I found it rather confusing that libpq looks up a symbol and then sets libpq-oauth's symbol to a pointer in libpq's namespace. > > But more fundamentally, are we actually using > > pg_g_threadlock anywhere? I removed all the releant code and the tests still > > seem to pass? > > If you have an older Curl installation, it is used. Newer libcurls > don't need it. Oh, wow. We hide the references to pg_g_threadlock behind a friggin macro? That's just ... Not your fault, I know... > A future simplification could be to pull the use of the threadlock > back into libpq, and have it perform a one-time > dlopen-plus-Curl-initialization under the lock... That would also get > rid of the dlerror() thread safety problems. But that's an awful lot > of moving parts under a mutex, which makes me a little nervous. I still think we should simply reject at configure time if curl init isn't threadsafe ;) > On Mon, Apr 7, 2025 at 7:43 AM Andres Freund <and...@anarazel.de> wrote: > > > Yes, this is correct. We want a shared module, not a shared library, in > > > meson parlance. > > > > It's not entirely obvious to me that we do want that. > > > > There recently was a breakage of building with PG on macos with meson, due > > to > > the meson folks implementing a feature request to move away from using > > bundles, as > > 1) bundles apparently aren't supported on iOS > > 2) there apparently aren't any restrictions left that require using bundles, > > and there haven't been for a while. > > Could you explain how this is related to .app bundles? I thought I was > just building a standard dylib. The other kind of bundles (what on earth apple was thinking with the naming here I don't know). Stuff liked with ld -bundle. > > > I don't know whether we need an exports file for libpq-oauth. The other > > > shared modules don't provide an export file, and I'm not sure whether this > > > is even supported for shared modules. I guess it doesn't hurt? > > > > It does seem just using PGDLLEXPORT would suffice here. > > My motivation was to strictly identify the ABI that we intend libpq to > use, to try to future-proof things for everybody. Especially since > we're duplicating functions from libpq that we'd rather not be. (The > use of RTLD_LOCAL maybe makes that more of a belt-and-suspenders > thing.) PGDLLEXPORT serves that purpose too, fwiw. These days we use compiler flags that restrict function visibility of everything not annotated with PGDLLEXPORT. However - that's all in c.h, port/win32.h,port/cygwin.h, , which libpq headers might not want to include. > Are there any downsides to the exports file? I think it's fine either way. Greetings, Andres Freund
