Обсуждение: PGDLLEXPORTing all GUCs?
Hi all As part of development on BDR the issue of GUC globals not being PGDLLEXPORT'ed has been run into a few times. Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic concerns? Barring objections I'll post a patch to do this tomorrow. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer <craig@2ndquadrant.com> writes: > Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic > concerns? That seems morally equivalent to "is there a reason not to make every static variable global?". IOW, no, I don't accept this proposition. Every time we DLLEXPORT some variable, we lose the freedom to redefine it later. So DLLEXPORT'ing GUCs should be on a case by case basis, just as for any other variable. In some cases it might be smarter to export a wrapper function. regards, tom lane
On 2014-05-07 09:35:06 -0400, Tom Lane wrote: > Craig Ringer <craig@2ndquadrant.com> writes: > > Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic > > concerns? > > That seems morally equivalent to "is there a reason not to make every > static variable global?". > > IOW, no, I don't accept this proposition. Every time we DLLEXPORT some > variable, we lose the freedom to redefine it later. So DLLEXPORT'ing GUCs > should be on a case by case basis, just as for any other variable. In > some cases it might be smarter to export a wrapper function. I think what Craig actually tries to propose is to mark all GUCs currently exported in headers PGDLLIMPORT. Currently it's easy to have extensions that work on sane systems but not windows. If they're already exposed in headers I don't think changes get any harder just because thy also can get used on windows... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 05/07/2014 09:45 PM, Andres Freund wrote: > I think what Craig actually tries to propose is to mark all GUCs > currently exported in headers PGDLLIMPORT. Currently it's easy to have > extensions that work on sane systems but not windows. If they're already > exposed in headers I don't think changes get any harder just because thy > also can get used on windows... Yes, rather. Exporting GUCs that're currently static wouldn't make sense. I'm just taking about making what works on !windows work on Windows. If a GUC is declared extern in a header, it should be PGDLLIMPORT. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2014-05-07 09:35:06 -0400, Tom Lane wrote: >> Craig Ringer <craig@2ndquadrant.com> writes: >>> Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic >>> concerns? >> That seems morally equivalent to "is there a reason not to make every >> static variable global?". > I think what Craig actually tries to propose is to mark all GUCs > currently exported in headers PGDLLIMPORT. There are few if any GUCs that aren't exposed in headers, just so that guc.c can communicate with the owning modules. That doesn't mean that we want everybody in the world messing with them. To my mind, we PGDLLEXPORT some variable only after deciding that yeah, we're okay with having third-party modules touching that. Craig's proposal is to remove human judgement from that process. regards, tom lane
Tom Lane-2 wrote > Andres Freund < > andres@ > > writes: >> On 2014-05-07 09:35:06 -0400, Tom Lane wrote: >>> Craig Ringer < > craig@ > > writes: >>>> Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic >>>> concerns? > >>> That seems morally equivalent to "is there a reason not to make every >>> static variable global?". > >> I think what Craig actually tries to propose is to mark all GUCs >> currently exported in headers PGDLLIMPORT. > > There are few if any GUCs that aren't exposed in headers, just so that > guc.c can communicate with the owning modules. That doesn't mean that > we want everybody in the world messing with them. > > To my mind, we PGDLLEXPORT some variable only after deciding that yeah, > we're okay with having third-party modules touching that. Craig's > proposal is to remove human judgement from that process. So third-party modules that use GUC's that are not PGDLLEXPORT are doing so improperly - even if it works for them because they only care/test non-Windows platforms? David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/PGDLLEXPORTing-all-GUCs-tp5802901p5802955.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
On 2014-05-07 10:29:36 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2014-05-07 09:35:06 -0400, Tom Lane wrote: > >> Craig Ringer <craig@2ndquadrant.com> writes: > >>> Is there any reason _not_ to PGDLLEXPORT all GUCs, other than cosmetic > >>> concerns? > > >> That seems morally equivalent to "is there a reason not to make every > >> static variable global?". > > > I think what Craig actually tries to propose is to mark all GUCs > > currently exported in headers PGDLLIMPORT. > > There are few if any GUCs that aren't exposed in headers, just so that > guc.c can communicate with the owning modules. That doesn't mean that > we want everybody in the world messing with them. > > To my mind, we PGDLLEXPORT some variable only after deciding that yeah, > we're okay with having third-party modules touching that. Craig's > proposal is to remove human judgement from that process. It's just levelling the planefield between platforms. If I had an idea how I'd still like to just PGDDLIMPORT *all* 'export'ed variables on windows. The problem is that there's lot of variables which aren't exported and which we'll only discover after the release. Just look at what e.g. postgres_fdw needed. It's not particularly unlikely that others fdws need some of those as well. But they can't change the release at the same time. If we want to declare variables off limits to extension/external code we need a solution that works on !windows as well. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2014-05-07 10:29:36 -0400, Tom Lane wrote: >> To my mind, we PGDLLEXPORT some variable only after deciding that yeah, >> we're okay with having third-party modules touching that. Craig's >> proposal is to remove human judgement from that process. > It's just levelling the planefield between platforms. If I had an idea > how I'd still like to just PGDDLIMPORT *all* 'export'ed variables on > windows. > The problem is that there's lot of variables which aren't exported and > which we'll only discover after the release. Just look at what > e.g. postgres_fdw needed. It's not particularly unlikely that others > fdws need some of those as well. But they can't change the release at > the same time. [ shrug... ] That problem is uncorrelated with GUC status, however. If that's your argument for a patch, then the patch should DLLEXPORT *every single non-static variable*. Which is a discussion we've already had, and rejected. I'd not be against an automatic mechanism for that, and indeed put considerable work into trying to make it happen a few months ago. But I'll resist wholesale cluttering of the source code with those markers. As long as we have to have them, I think we should use them in the way I outlined, that we mark only variables that are "considered okay to access". In fact, GUCs are exactly the *last* variables that should get marked that way automatically, because so many of them are global only because of the need for guc.c to communicate with the owning module, not because we want anything else touching them. regards, tom lane
On Wed, May 7, 2014 at 11:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@2ndquadrant.com> writes: >> On 2014-05-07 10:29:36 -0400, Tom Lane wrote: >>> To my mind, we PGDLLEXPORT some variable only after deciding that yeah, >>> we're okay with having third-party modules touching that. Craig's >>> proposal is to remove human judgement from that process. > >> It's just levelling the planefield between platforms. If I had an idea >> how I'd still like to just PGDDLIMPORT *all* 'export'ed variables on >> windows. >> The problem is that there's lot of variables which aren't exported and >> which we'll only discover after the release. Just look at what >> e.g. postgres_fdw needed. It's not particularly unlikely that others >> fdws need some of those as well. But they can't change the release at >> the same time. > > [ shrug... ] That problem is uncorrelated with GUC status, however. > If that's your argument for a patch, then the patch should DLLEXPORT > *every single non-static variable*. Which is a discussion we've already > had, and rejected. > > I'd not be against an automatic mechanism for that, and indeed put > considerable work into trying to make it happen a few months ago. But > I'll resist wholesale cluttering of the source code with those markers. > As long as we have to have them, I think we should use them in the way > I outlined, that we mark only variables that are "considered okay to > access". In fact, GUCs are exactly the *last* variables that should get > marked that way automatically, because so many of them are global only > because of the need for guc.c to communicate with the owning module, > not because we want anything else touching them. I don't accept this argument. In EnterpriseDB's Advanced Server fork of PostgreSQL, we've marked a bunch of extra things PGDLLEXPORT precisely because we have external modules that need access to them. Of course, what that means is that when PostgreSQL changes things around in a future release, we have to go revise those external modules as well. However, that just doesn't happen often enough to actually pose a significant problem for us. It's not really accurate to think that people are only going to rely on the things that we choose to PGDLLEXPORT. It's more accurate to think that when we don't mark things PGDLLEXPORT, we're forcing people to decide between (1) giving up on writing their extension, (2) having that extension not work on Windows, (3) submitting a patch to add a PGDLLEXPORT marking and waiting an entire release cycle for that to go G.A., and then still not being able to support older versions, or (4) forking PostgreSQL. That's an unappealing list of options. I would not go so far as to say that we should PGDLLEXPORT absolutely every non-static variable. But I think adding those markings to every GUC we've got is perfectly reasonable. I am quite sure that the 2ndQuadrant folks know that they'll be on the hook to update any code they write that uses those variables if and when a future version of PostgreSQL whacks things around. But that's not a new problem - the same thing happens when a function signature changes, or when a variable that does happen to have a PGDLLEXPORT marking changes. And at least in my experience it's also not a large problem. The amount of time EnterpriseDB spends updating our (large!) amount of proprietary code in response to such changes is a small percentage of our overall development time. Enabling extensibility is a far more important goal than keeping people from committing to interfaces that may change in the future, especially since the latter is a losing battle anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I don't accept this argument. In EnterpriseDB's Advanced Server fork > of PostgreSQL, we've marked a bunch of extra things PGDLLEXPORT > precisely because we have external modules that need access to them. Well, that's an argument for marking every darn global variable as PGDLLEXPORT. But it's *not* an argument for marking GUCs in particular that way. In particular, you are conveniently ignoring the point that GUCs are much more likely to be global as an artifact of the way guc.c is modularized than because we actually think they should be globally accessible. If Craig has a concrete argument why all GUCs should be accessible to external modules, then let's see it (after which we'd better debate exposing the few that are in fact static in guc.c). Or if you want to hang your hat on the platform-leveling argument, then we should be re-debating exporting *all* global variables. But as far as the actually proposed patch goes, all I'm hearing is very confused thinking. regards, tom lane
On 2014-05-07 12:21:55 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > I don't accept this argument. In EnterpriseDB's Advanced Server fork > > of PostgreSQL, we've marked a bunch of extra things PGDLLEXPORT > > precisely because we have external modules that need access to them. > > Well, that's an argument for marking every darn global variable as > PGDLLEXPORT. But it's *not* an argument for marking GUCs in particular > that way. In particular, you are conveniently ignoring the point that > GUCs are much more likely to be global as an artifact of the way guc.c > is modularized than because we actually think they should be globally > accessible. GUCs in general are user configurable things. So it's not particularly unreasonable to assume that a significant fraction of them are of interest to extensions. And it's not like exporting them gives way to many additional dangers - they already can be overwritten. In fact, I am pretty sure that nearly all of these cases are about *reading* the underlying variable not writing them. It's pretty darn less convenient and far slower to get the config variable as text and then convert it to the underlying type. > (after which we'd better debate exposing the few that are in fact > static in guc.c). I plan to do that for most of them - completely independently of this topic. I think 'export'ing a variable in several files is a horrible idea. > Or if you want > to hang your hat on the platform-leveling argument, then we should be > re-debating exporting *all* global variables. Yes. I am wondering whether that's not the most sensible course. It's a pita, but essentially we'd have to do a global s/export/pg_export/ in the headers. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I don't accept this argument. In EnterpriseDB's Advanced Server fork >> of PostgreSQL, we've marked a bunch of extra things PGDLLEXPORT >> precisely because we have external modules that need access to them. > > Well, that's an argument for marking every darn global variable as > PGDLLEXPORT. But it's *not* an argument for marking GUCs in particular > that way. In particular, you are conveniently ignoring the point that > GUCs are much more likely to be global as an artifact of the way guc.c > is modularized than because we actually think they should be globally > accessible. > > If Craig has a concrete argument why all GUCs should be accessible > to external modules, then let's see it (after which we'd better debate > exposing the few that are in fact static in guc.c). Or if you want > to hang your hat on the platform-leveling argument, then we should be > re-debating exporting *all* global variables. But as far as the actually > proposed patch goes, all I'm hearing is very confused thinking. I think there's actually a very good reason to think that GUCs are good candidates for this treatment, which is that, by definition, the GUC is a public interface: you can change it with a SET command. It's certainly easier to imagine an extension wanting access to update_process_title than, say, criticalRelcachesBuilt. But maybe you're right and we should revisit the idea of exposing everything. A quick grep through src/include suggests that GUCs are a big percentage of what's not marked PGDLLEXPORT anyway, and the other stuff that's in there is stuff like PgStartTime and PostmasterPid which hardly seem like silly things to expose. I certainly think we should err on the side of exposing stuff that people think might be useful rather than pretending that we can stop them from using symbols by refusing to PGDLLEXPORT them. We can't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> If Craig has a concrete argument why all GUCs should be accessible >> to external modules, then let's see it (after which we'd better debate >> exposing the few that are in fact static in guc.c). > I think there's actually a very good reason to think that GUCs are > good candidates for this treatment, which is that, by definition, the > GUC is a public interface: you can change it with a SET command. Sure, and we provide public APIs for accessing/setting GUCs. The SET side of that is most emphatically *not* "just set the C variable". Yeah, you can get away with reading them like that, assuming you want the internal representation not the user-visible one. In any case, I've not heard the use-case why all (and only) GUCs might need to be readable in that way. Again, I'm not arguing against a proposal that we should automatically export all globally-declared variables for platform-levelling reasons. I *am* saying that I find a proposal to do that just to GUCs to be unsupported by any argument made so far. regards, tom lane
On 2014-05-07 13:08:52 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> If Craig has a concrete argument why all GUCs should be accessible > >> to external modules, then let's see it (after which we'd better debate > >> exposing the few that are in fact static in guc.c). > > > I think there's actually a very good reason to think that GUCs are > > good candidates for this treatment, which is that, by definition, the > > GUC is a public interface: you can change it with a SET command. > > Sure, and we provide public APIs for accessing/setting GUCs. As strings. Not a useful representation for C code... And to avoid units getting tacked on you need to first get the config option number, then allocate an array on the stack, call GetConfigOptionByNum(), then parse the result into the underlying type. Meh. > The SET > side of that is most emphatically *not* "just set the C variable". > Yeah, you can get away with reading them like that, assuming you want > the internal representation not the user-visible one. In any case, > I've not heard the use-case why all (and only) GUCs might need to be > readable in that way. I think you're making up the 'and only' part. There's lots of variables that should/need to be exported. Just look at the amount of mess you cleaned up with variou extensions not actually working on windows... Last time round you argued against exporting all variables. So Craig seems to have choosen a subset that's likely to be needed. > Again, I'm not arguing against a proposal that we should automatically > export all globally-declared variables for platform-levelling reasons. > I *am* saying that I find a proposal to do that just to GUCs to be > unsupported by any argument made so far. Well, then let's start discussing that proposal then. I propose having defining a 'pg_export' macro that's suitably defined by the buildsystem. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, May 7, 2014 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> If Craig has a concrete argument why all GUCs should be accessible >>> to external modules, then let's see it (after which we'd better debate >>> exposing the few that are in fact static in guc.c). > >> I think there's actually a very good reason to think that GUCs are >> good candidates for this treatment, which is that, by definition, the >> GUC is a public interface: you can change it with a SET command. > > Sure, and we provide public APIs for accessing/setting GUCs. The SET > side of that is most emphatically *not* "just set the C variable". > Yeah, you can get away with reading them like that, assuming you want > the internal representation not the user-visible one. In any case, > I've not heard the use-case why all (and only) GUCs might need to be > readable in that way. My experience is that GUCs are a common thing to want expose to extensions, and that C code usually wants the internal form, not the string form. I'm not arguing that nothing else needs to be exposed, but if there's a better argument possible for exposing the GUC variables than the fact that a bunch of people with experience developing PostgreSQL extensions view that as a real need, I can't think what it is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 05/08/2014 12:21 AM, Tom Lane wrote: > If Craig has a concrete argument why all GUCs should be accessible > to external modules, then let's see it Because they already are. The only difference here is that that access works only on !windows. I agree (strongly) that we should have a better defined API in terms of what is "accessible to external modules" and what is not. However, we don't, as you stressed just that in a prior discussion when I raised the idea of using -fvisbility=hidden to limit access to some symbols. Given that we don't have any kind of exernal vs internal API division, why pretend we do just for one platform? As for just GUCs: I suggested GUCs because GUCs are what's been coming up repeatedly as an actual practical issue. I'd be quite happy to PGDLLEXPORT all extern vars, but I was confident that'd be rejected for aesthetic reasons, and thought that exporting all GUCs would be a reasonable compromise. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Craig Ringer <craig@2ndquadrant.com> writes: > On 05/08/2014 12:21 AM, Tom Lane wrote: >> If Craig has a concrete argument why all GUCs should be accessible >> to external modules, then let's see it > As for just GUCs: I suggested GUCs because GUCs are what's been coming > up repeatedly as an actual practical issue. Meh. A quick look through the commit logs says that GUC variables are not more than 50% of what we've had to PGDLLIMPORT'ify in the past year or two. Maybe that's different from 2ndQuadrant's internal experience, but then you've not showed us the use-case driving your changes. > I'd be quite happy to > PGDLLEXPORT all extern vars, but I was confident that'd be rejected for > aesthetic reasons, and thought that exporting all GUCs would be a > reasonable compromise. From the aesthetic standpoint, what I'd like is to not have to blanket our source code with Windows-isms. But I guess I can't have that. regards, tom lane
On 05/08/2014 10:53 AM, Tom Lane wrote: > Craig Ringer <craig@2ndquadrant.com> writes: >> On 05/08/2014 12:21 AM, Tom Lane wrote: >>> If Craig has a concrete argument why all GUCs should be accessible >>> to external modules, then let's see it > >> As for just GUCs: I suggested GUCs because GUCs are what's been coming >> up repeatedly as an actual practical issue. > > Meh. A quick look through the commit logs says that GUC variables are not > more than 50% of what we've had to PGDLLIMPORT'ify in the past year or > two. Maybe that's different from 2ndQuadrant's internal experience, > but then you've not showed us the use-case driving your changes. That's because the use case isn't that interesting, really, only the hiccups it's caused. I'd just as happily mark all extern vars PGDLLIMPORT, and only suggested focusing on GUCs because I didn't expect to get far with what I really thought was best. Re your concerns with exposing GUCs that should by rights be internal to the wider world, it'd be interesting to mark functions and vars as something like PGINTERNAL, to expand to: __attribute__((visibility ("hidden")) on gcc, so they're just not available for linkage in extensions. That's a weaker form of using -fvisibility=hidden, where we explicitly say "this is private" rather than treating everything as private unless explicitly marked public, which has already been rejected. Right now they're already exported for !windows, and while it's IMO a bug to have that difference for windows, it doesn't mean the correct answer is to export for all. If we're confident it won't break anything interesting it'd be OK to instead say "unexport on !windows too". In terms of ugliness, would you be happier using a pg_extern instead of extern, where we: #ifdef WIN32 #define pg_extern extern PGDLLIMPORT #else #define pg_extern extern #endif ? That makes it easier to pretend that there's nothing windows-y going on - and despite appearances, I'm also pretty keen not to have to think about that platform's linkage horrors when I don't have to. However, it also makes backpatching ickier. >> I'd be quite happy to >> PGDLLEXPORT all extern vars, but I was confident that'd be rejected for >> aesthetic reasons, and thought that exporting all GUCs would be a >> reasonable compromise. > > From the aesthetic standpoint, what I'd like is to not have to blanket > our source code with Windows-isms. But I guess I can't have that. I'd rather prefer that as well, but without the ability to go knocking at Redmond and introduce them to ELF and sane linkage, I don't think any of us are going to get it. If it weren't for backbranches etc the first thing I'd do to make it less ugly personally would be to rename PGDLLIMPORT as PGEXPORT and BUILDING_DLL as BUILDING_POSTGRES . The current names are unfortunate. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, May 8, 2014 at 12:19 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > In terms of ugliness, would you be happier using a pg_extern instead of > extern, where we: > > #ifdef WIN32 > #define pg_extern extern PGDLLIMPORT > #else > #define pg_extern extern > #endif > > ? I personally would not be happier with that. extern can be applied to function prototypes, not just variables, and even to function definitions. When I see PGDLLIMPORT, I think, oh look, this is some kind of magic pixie dust that Windows requires us to sprinkle on our variables. When I see pg_extern, I think, oh, this is the PostgreSQL version of extern, and it's not, really. But I can live with it if it makes other people sufficiently happier. One way or another, I really do feel very strongly that we should push forward with broader PGDLLIMPORT-ification. My experience mirrors Craig's: this is a very useful thing for extension developers. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-05-08 07:56:46 -0400, Robert Haas wrote: > On Thu, May 8, 2014 at 12:19 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > > In terms of ugliness, would you be happier using a pg_extern instead of > > extern, where we: > > > > #ifdef WIN32 > > #define pg_extern extern PGDLLIMPORT > > #else > > #define pg_extern extern > > #endif > > > > ? > > I personally would not be happier with that. extern can be applied to > function prototypes, not just variables, and even to function > definitions. When I see PGDLLIMPORT, I think, oh look, this is some > kind of magic pixie dust that Windows requires us to sprinkle on our > variables. When I see pg_extern, I think, oh, this is the PostgreSQL > version of extern, and it's not, really. Well, it's actually "helpful" in some sense for functions too (one trampoline less on windows). And given how postgres uses externs I think it matches well with just PGDLLIMPORT everything. > But I can live with it if it makes other people sufficiently happier. > One way or another, I really do feel very strongly that we should push > forward with broader PGDLLIMPORT-ification. My experience mirrors > Craig's: this is a very useful thing for extension developers. Yea. I have been yelled at by jenkins far too many times. Exporting all variables seems like the only way to significantly reduce the need for !windows developers needing to care about windows. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services