Re: PGDLLEXPORTing all GUCs?

Lists: pgsql-hackers
From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 08:44:05
Message-ID: 5369F255.7000008@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Craig Ringer <craig(at)2ndquadrant(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 13:35:06
Message-ID: 25882.1399469706@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Craig Ringer <craig(at)2ndquadrant(dot)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


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 13:45:00
Message-ID: 20140507134500.GC13397@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-05-07 09:35:06 -0400, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)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


From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 14:26:21
Message-ID: 536A428D.50805@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 14:29:36
Message-ID: 27201.1399472976@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-07 09:35:06 -0400, Tom Lane wrote:
>> Craig Ringer <craig(at)2ndquadrant(dot)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


From: David G Johnston <david(dot)g(dot)johnston(at)gmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 14:58:29
Message-ID: 1399474709490-5802955.post@n5.nabble.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane-2 wrote
> Andres Freund &lt;

> andres@

> &gt; writes:
>> On 2014-05-07 09:35:06 -0400, Tom Lane wrote:
>>> Craig Ringer &lt;

> craig@

> &gt; 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.


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 15:04:49
Message-ID: 20140507150449.GF13397@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-05-07 10:29:36 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2014-05-07 09:35:06 -0400, Tom Lane wrote:
> >> Craig Ringer <craig(at)2ndquadrant(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 15:19:11
Message-ID: 28334.1399475951@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Andres Freund <andres(at)2ndquadrant(dot)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


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 15:56:25
Message-ID: CA+TgmoYK8nGZ8bPaCs+8ZPBZ-us3SCQppm742-xpHHPxS4+wMQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, May 7, 2014 at 11:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 16:21:55
Message-ID: 29777.1399479715@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)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


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 16:42:03
Message-ID: 20140507164203.GG13397@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-05-07 12:21:55 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 16:44:36
Message-ID: CA+TgmoYgFqE=jQp7AuQOjBq4Km-LRBLjPGso1i7Mo07NEtDoxQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 17:08:52
Message-ID: 30958.1399482532@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 17:21:45
Message-ID: 20140507172145.GI13397@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-05-07 13:08:52 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-07 17:22:45
Message-ID: CA+Tgmobm46ueUEOnbh-5TX6r5ZBBRprhyAfcCRi3kbF=6GHaDA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, May 7, 2014 at 1:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, May 7, 2014 at 12:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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


From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-08 00:56:48
Message-ID: 536AD650.9000400@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Craig Ringer <craig(at)2ndquadrant(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-08 02:53:45
Message-ID: 17254.1399517625@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Craig Ringer <craig(at)2ndquadrant(dot)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


From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-08 04:19:26
Message-ID: 536B05CE.3040403@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 05/08/2014 10:53 AM, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)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


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Craig Ringer <craig(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-08 11:56:46
Message-ID: CA+TgmobT3CBxZADHD4A-G778-Z7WOQLgDfDyg5oU2LOfVhXLOA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, May 8, 2014 at 12:19 AM, Craig Ringer <craig(at)2ndquadrant(dot)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


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PGDLLEXPORTing all GUCs?
Date: 2014-05-08 12:00:34
Message-ID: 20140508120034.GH30324@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-05-08 07:56:46 -0400, Robert Haas wrote:
> On Thu, May 8, 2014 at 12:19 AM, Craig Ringer <craig(at)2ndquadrant(dot)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