Обсуждение: Random PGDLLIMPORTing

Hi all

Noticed this while reading something unrelated

extern PGDLLIMPORT pid_t PostmasterPid;
extern bool IsPostmasterEnvironment;
extern PGDLLIMPORT bool IsUnderPostmaster;
extern bool IsBackgroundWorker;
extern PGDLLIMPORT bool IsBinaryUpgrade;

I don't see any sane reason for some of those to be PGDLLIMPORT but
not others. In particular it's pretty silly for IsBackgroundWorker not
to be PGDLLIMPORT.

Too trivial to be worth making an actual patch, it'd be more work to
apply it than edit directly.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Thu, 24 Nov 2016 15:01:33 +0100
Magnus Hagander <magnus@hagander.net> wrote:

> On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer <craig@2ndquadrant.com>
> wrote:
> 
> > Hi all
> >
> > Noticed this while reading something unrelated
> >
> > extern PGDLLIMPORT pid_t PostmasterPid;
> > extern bool IsPostmasterEnvironment;
> > extern PGDLLIMPORT bool IsUnderPostmaster;
> > extern bool IsBackgroundWorker;
> > extern PGDLLIMPORT bool IsBinaryUpgrade;
> >
> > I don't see any sane reason for some of those to be PGDLLIMPORT but
> > not others. In particular it's pretty silly for IsBackgroundWorker
> > not to be PGDLLIMPORT.
> >
> > Too trivial to be worth making an actual patch, it'd be more work to
> > apply it than edit directly.
> >  
> 
> My guess is that PGDLLIMPORT has been added explicitly when somebody
> needed it for something, without any actual thought. I can't say I
> see any reason not to export the other ones as well -- more that
> maybe there are even more that are needed?
> 

It worth checking actual variable definitions, not just declarations.
I've found recently, that at least in MSVC build system, only
initialized variables are included into postgres.def file, and so are
actually exported from the backend binary.

But in this case both IsPostmasterEnvironment and IsBackgroundWorker
are initialized. So probably they are not marked as importable only for
historic reason.

On Thu, Nov 24, 2016 at 11:01 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> My guess is that PGDLLIMPORT has been added explicitly when somebody needed
> it for something, without any actual thought. I can't say I see any reason
> not to export the other ones as well -- more that maybe there are even more
> that are needed?

Yes, more or less. The reasoning behind at least the PostmasterPID bit is that:
https://www.postgresql.org/message-id/CAB7nPqS_=14KRCDcH6NyRsW8c58J1cwXv6QCVS7YP-6tTHQ1nA@mail.gmail.com
That resulted in cac83219.

The other ones could perhaps be marked with PGDLLIMPORT. Now usually
we need a use-case behind this change, and there are not that many
hackers on Windows doing much plugin development.
-- 
Michael

On 25 November 2016 at 07:36, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 11:01 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> My guess is that PGDLLIMPORT has been added explicitly when somebody needed
>> it for something, without any actual thought. I can't say I see any reason
>> not to export the other ones as well -- more that maybe there are even more
>> that are needed?
>
> Yes, more or less. The reasoning behind at least the PostmasterPID bit is that:
> https://www.postgresql.org/message-id/CAB7nPqS_=14KRCDcH6NyRsW8c58J1cwXv6QCVS7YP-6tTHQ1nA@mail.gmail.com
> That resulted in cac83219.
>
> The other ones could perhaps be marked with PGDLLIMPORT. Now usually
> we need a use-case behind this change, and there are not that many
> hackers on Windows doing much plugin development.

Exactly, that's why nobody's been shouting yet.

The use case is is exactly the same as the use case for these vars
existing. IsBackgroundWorker in particular makes no sense to have at
all if it isn't exported, and the only reason nobody's complaining is
that nobody's building cassert binaries under Windows or has tried to
write anything that has behaviour that cares if it's in a bgworker or
not.

PGDLLIMPORT is free, so the question should be "is there a reason not
to add it here?".

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:
> PGDLLIMPORT is free, so the question should be "is there a reason not
> to add it here?".

TBH, my basic complaint about it is that I do not like Microsoft's tool
chain assuming that it's entitled to demand that people sprinkle
Microsoft-specific droppings throughout what would otherwise be platform
independent source code.

However, Victor Wagner's observation upthread is quite troubling:

>> It worth checking actual variable definitions, not just declarations.
>> I've found recently, that at least in MSVC build system, only
>> initialized variables are included into postgres.def file, and so are
>> actually exported from the backend binary.

If this is correct (don't ask me, I don't do Windows) then the issue is
not whether "PGDLLIMPORT is free".  This puts two separate source-code
demands on variables that we want to make available to extensions, neither
of which is practically checkable on non-Windows platforms.

I think that basically it's going to be on the heads of people who
want to work on Windows to make sure that things work on that platform.
That is the contract that every other platform under the sun understands,
but it seems like Windows people think it's on the rest of us to make
their platform work.  I'm done with that.
        regards, tom lane

On 25 November 2016 at 14:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> PGDLLIMPORT is free, so the question should be "is there a reason not
>> to add it here?".
>
> TBH, my basic complaint about it is that I do not like Microsoft's tool
> chain assuming that it's entitled to demand that people sprinkle
> Microsoft-specific droppings throughout what would otherwise be platform
> independent source code.

Yeah, I know how you feel. I'm not a fan myself, I just tolerate it as
one of those annoying things we must put up with, like Perl 5.8 for
ancient systems, not using C99, and !Linux/FBSD support. Which,
actually, we _do_ actively work for - take for example the atomics
work, which went to considerable lengths not to break weird niche
platforms.

> However, Victor Wagner's observation upthread is quite troubling:
>
>>> It worth checking actual variable definitions, not just declarations.
>>> I've found recently, that at least in MSVC build system, only
>>> initialized variables are included into postgres.def file, and so are
>>> actually exported from the backend binary.
>
> If this is correct (don't ask me, I don't do Windows) then the issue is
> not whether "PGDLLIMPORT is free".  This puts two separate source-code
> demands on variables that we want to make available to extensions, neither
> of which is practically checkable on non-Windows platforms.

I agree that, if true, that's a real concern and something that needs
addressing to avoid a growing maintenance burden from Windows.

> I think that basically it's going to be on the heads of people who
> want to work on Windows to make sure that things work on that platform.
> That is the contract that every other platform under the sun understands,
> but it seems like Windows people think it's on the rest of us to make
> their platform work.  I'm done with that.

Well, I'm trying to be one of those "Windows people" to the degree of
spotting and addressing issues. Like this one. But it's not worth
arguing this one if it's more than a trivial "meh, done" fix, since
it's likely to only upset code that's testing assertions (like BDR or
pglogical).

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services