Re: [HACKERS] Random PGDLLIMPORTing
On Fri, Nov 25, 2016 at 9:54 AM, Craig Ringer wrote: > On 25 November 2016 at 14:16, Tom Lane wrote: > > Craig Ringer 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. > It would probably not be very hard to create a test that just scans the sourcecode for PGDLLIMPORT:ed variables and generates a C file that links to them all, tries to build that and sees if it blows up. If I understand the issue correctly, that should fail in these cases. So we could make that a platform specific test that runs, and have the buildfarm run it so we'd at least detect such problems early. Or am I missing something? > > 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). > > So when we *do* identify these places, through projects like that or just general complaints, do we see any actual risk in backpatching such additions? As long as the linking is done by name and not by number, it should be fully safe, right? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Random PGDLLIMPORTing
On 25 November 2016 at 14:16, Tom Lane wrote: > Craig Ringer 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Random PGDLLIMPORTing
Craig Ringer 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Random PGDLLIMPORTing
On 25 November 2016 at 07:36, Michael Paquier wrote: > On Thu, Nov 24, 2016 at 11:01 PM, Magnus Hagander wrote: >> On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer 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-6tthq...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Random PGDLLIMPORTing
On Thu, Nov 24, 2016 at 11:01 PM, Magnus Hagander wrote: > On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer 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-6tthq...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Random PGDLLIMPORTing
On Thu, 24 Nov 2016 15:01:33 +0100 Magnus Hagander wrote: > On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer > 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Random PGDLLIMPORTing
On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer 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? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Random PGDLLIMPORTing
On Thu, Nov 24, 2016 at 2:58 PM, Craig Ringer 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. > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
