Thread: Add missing PGDLLIMPORT markings
I ran src/tools/mark_pgdllimport.pl and it detected a few new global
variables with missing markings. See attached patch. Please point out
if any of these should not be marked or if they are special cases in
some other way. I'm Cc'ing various people involved with that new code.
Btw., this new variable in memutils.h
extern dsa_area *area;
could probably do with a less generic name?
Attachment
> On 9 Apr 2025, at 12:02, Peter Eisentraut <peter@eisentraut.org> wrote: -extern const pg_be_sasl_mech pg_be_oauth_mech; +extern PGDLLIMPORT const pg_be_sasl_mech pg_be_oauth_mech; +1 on this. > Btw., this new variable in memutils.h > > extern dsa_area *area; > > could probably do with a less generic name? Good point, unless objected to I'll apply the attached renaming which then also contain the PGDLLIMPORT addition. -- Daniel Gustafsson
Attachment
On Wed, Apr 9, 2025 at 4:48 AM Daniel Gustafsson <daniel@yesql.se> wrote: > -extern const pg_be_sasl_mech pg_be_oauth_mech; > +extern PGDLLIMPORT const pg_be_sasl_mech pg_be_oauth_mech; > +1 on this. LGTM, too. Thanks! --Jacob
Hi, On 2025-04-09 12:02:52 +0200, Peter Eisentraut wrote: > I ran src/tools/mark_pgdllimport.pl and it detected a few new global > variables with missing markings. See attached patch. Please point out if > any of these should not be marked or if they are special cases in some other > way. I'm Cc'ing various people involved with that new code. FWIW, the AIO ones really don't make sense to make public - the only reason for those variables to exists is so they can be put into an array of callbacks. There's no way an extension could ever benefit from them. But I guess we don't really have a way to tell mark_pgdllimport.pl that. I did mark the internal AIO variables that maybe kinda somewhat insanely used by an extension as PGDLLIMPORT (c.f. aio_internal.h)... Greetings, Andres Freund
On Wed, Apr 9, 2025 at 11:28 AM Andres Freund <andres@anarazel.de> wrote: > FWIW, the AIO ones really don't make sense to make public - the only reason > for those variables to exists is so they can be put into an array of > callbacks. There's no way an extension could ever benefit from them. But I > guess we don't really have a way to tell mark_pgdllimport.pl that. I'm not here to say that you're wrong, but this kind of argument is exactly why we didn't use to mark a bunch of things PGDLLIMPORT that, as it turned out, extension developers actually wanted to use. I don't think we should go back to the bad old days where we litigated every case of marking something PGDLLIMPORT or not unless we have an extremely good reason for so doing. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On April 9, 2025 12:58:23 PM EDT, Robert Haas <robertmhaas@gmail.com> wrote: >On Wed, Apr 9, 2025 at 11:28 AM Andres Freund <andres@anarazel.de> wrote: >> FWIW, the AIO ones really don't make sense to make public - the only reason >> for those variables to exists is so they can be put into an array of >> callbacks. There's no way an extension could ever benefit from them. But I >> guess we don't really have a way to tell mark_pgdllimport.pl that. > >I'm not here to say that you're wrong, but this kind of argument is >exactly why we didn't use to mark a bunch of things PGDLLIMPORT that, >as it turned out, extension developers actually wanted to use. > >I don't think we should go back to the bad old days where we litigated >every case of marking something PGDLLIMPORT or not unless we have an >extremely good reason for so doing. Yeah, that's why I guessed that we will have to just mark these. Fwiw, just marking everything is far from free. We prevent optimizations the compiler could make otherwise. Except of coursethat we have implicitly always done so in the main binary on ELF platforms, so "PGDLLIMPORT everything" kind of justremoved the portability hazard. I think eventually we ought to change the default visibility in the main binary to hidden, just like we do for extensions.Then the windows/everything else behavior difference vanishes and we actually get the benefit of being more restrictiveon common platforms. I think we still ought to default to exporting symbols in such a world, just not alwaysdo so. We probably should just require all externs are either marked "server only" or "visible to extensions", neverjust a plain extern. Greetings, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Apr 9, 2025 at 1:10 PM Andres Freund <andres@anarazel.de> wrote: > I think eventually we ought to change the default visibility in the main binary to hidden, just like we do for extensions.Then the windows/everything else behavior difference vanishes and we actually get the benefit of being more restrictiveon common platforms. I think we still ought to default to exporting symbols in such a world, just not alwaysdo so. We probably should just require all externs are either marked "server only" or "visible to extensions", neverjust a plain extern. The issue we're going to run into is that a lot of extension authors like to call things that the core developers probably think they shouldn't. If we lock it down, we'll either be breaking a bunch of extensions that are doing sneaky things that somebody has managed to make work ... or we're going to be committing to an API that we don't really want to support. To be clear, I'm not saying you're wrong, just that it's going to be hard to change anything without making somebody unhappy. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> The issue we're going to run into is that a lot of extension authors
> like to call things that the core developers probably think they
> shouldn't. If we lock it down, we'll either be breaking a bunch of
> extensions that are doing sneaky things that somebody has managed to
> make work ... or we're going to be committing to an API that we don't
> really want to support.
> To be clear, I'm not saying you're wrong, just that it's going to be
> hard to change anything without making somebody unhappy.
Yeah. To do anything else than "export everything", we'd have to
engage in a long hard slog of negotiations over what we want to treat
as exported API and what we don't. From past experience with adding
PGDLLIMPORT markings piecemeal, that process would be never-ending,
because there would always be someone asking for access to something
else.
I don't really want to go back there. The current policy is
effectively that it's on extension developers to deal with it
when we change an API they depended on, and I'm content with that.
I don't buy that arguments like "maybe the compiler could
micro-optimize this a bit better if it weren't exported" should
drive our decision-making in this area.
regards, tom lane
> On 9 Apr 2025, at 13:48, Daniel Gustafsson <daniel@yesql.se> wrote: >> On 9 Apr 2025, at 12:02, Peter Eisentraut <peter@eisentraut.org> wrote: >> could probably do with a less generic name? > > Good point, unless objected to I'll apply the attached renaming which then also > contain the PGDLLIMPORT addition. Done. -- Daniel Gustafsson
On 09.04.25 12:02, Peter Eisentraut wrote: > I ran src/tools/mark_pgdllimport.pl and it detected a few new global > variables with missing markings. See attached patch. Please point out > if any of these should not be marked or if they are special cases in > some other way. I'm Cc'ing various people involved with that new code. I have committed the remaining ones.