Horton

Thread

Commits

  1. Add missing PGDLLIMPORT markings

  2. Rename global variable backing DSA area

  1. Add missing PGDLLIMPORT markings

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-09T10:02:52Z

    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?

  2. Re: Add missing PGDLLIMPORT markings

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-09T11:48:06Z

    > 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

  3. Re: Add missing PGDLLIMPORT markings

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-09T12:30:40Z

    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

  4. Re: Add missing PGDLLIMPORT markings

    Andres Freund <andres@anarazel.de> — 2025-04-09T15:27:58Z

    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

  5. Re: Add missing PGDLLIMPORT markings

    Robert Haas <robertmhaas@gmail.com> — 2025-04-09T16:58:23Z

    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

  6. Re: Add missing PGDLLIMPORT markings

    Andres Freund <andres@anarazel.de> — 2025-04-09T17:10:09Z

    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 course that we have implicitly always done so in the main binary on ELF platforms, so "PGDLLIMPORT everything" kind of just removed 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 restrictive on common platforms.  I think we  still ought to default to exporting symbols in such a world, just not always do so.  We probably should just require all externs are either marked "server only" or "visible to extensions", never just a plain extern.

Greetings, 

Andres 


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  7. Re: Add missing PGDLLIMPORT markings

    Robert Haas <robertmhaas@gmail.com> — 2025-04-10T16:03:00Z

    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 restrictive on common platforms.  I think we  still ought to default to exporting symbols in such a world, just not always do so.  We probably should just require all externs are either marked "server only" or "visible to extensions", never just 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

  8. Re: Add missing PGDLLIMPORT markings

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-10T16:19:41Z

    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

  9. Re: Add missing PGDLLIMPORT markings

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-10T22:38:02Z

    > 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

  10. Re: Add missing PGDLLIMPORT markings

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-11T07:08:06Z

    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.