Mark all GUC variable as PGDLLIMPORT

On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote:

This topic has been raised multiple time over the years, and I don't see any
objection to add such an annotation at least for all GUC variables (either the
direct variables or the indirect variables set during the hook execution), so
PFA a patch that takes care of all the GUC.

I don't now if that's still an option at that point, but backporting to at
least pg14 if that patch is accepted would be quite helpful.

These are usually just applied on HEAD, and on a parameter-basis based
on requests from extension authors. If you wish to make your
extensions able to work on Windows, that's a good idea, but I would
recommend to limit this exercise to what's really necessary for your
purpose.
--
Michael

On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote:

On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote:

This topic has been raised multiple time over the years, and I don't see any
objection to add such an annotation at least for all GUC variables (either the
direct variables or the indirect variables set during the hook execution), so
PFA a patch that takes care of all the GUC.

I don't now if that's still an option at that point, but backporting to at
least pg14 if that patch is accepted would be quite helpful.

These are usually just applied on HEAD

Yeah but 14 isn't released yet, and this is a really low risk change.

, and on a parameter-basis based
on requests from extension authors. If you wish to make your
extensions able to work on Windows, that's a good idea, but I would
recommend to limit this exercise to what's really necessary for your
purpose.

I disagree. For random global variables I agree that we shouldn't mark them
all blindly, but for GUCs it's pretty clear that they're intended to be
accessible from any caller, including extensions. Why treating Windows as a
second-class citizen, especially when any change can only be used a year after
someone complained?

ne 22. 8. 2021 v 14:08 odesílatel Julien Rouhaud <rjuju123@gmail.com>
napsal:

On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote:

On Sun, Aug 22, 2021 at 04:10:33PM +0800, Julien Rouhaud wrote:

This topic has been raised multiple time over the years, and I don't

see any

objection to add such an annotation at least for all GUC variables

(either the

direct variables or the indirect variables set during the hook

execution), so

PFA a patch that takes care of all the GUC.

I don't now if that's still an option at that point, but backporting

to at

least pg14 if that patch is accepted would be quite helpful.

These are usually just applied on HEAD

Yeah but 14 isn't released yet, and this is a really low risk change.

, and on a parameter-basis based
on requests from extension authors. If you wish to make your
extensions able to work on Windows, that's a good idea, but I would
recommend to limit this exercise to what's really necessary for your
purpose.

I disagree. For random global variables I agree that we shouldn't mark
them
all blindly, but for GUCs it's pretty clear that they're intended to be
accessible from any caller, including extensions. Why treating Windows as
a
second-class citizen, especially when any change can only be used a year
after
someone complained?

I had few problems with access with these variables too when I worked on
orafce.

Is true, so it increases differences between Windows and Unix, and fixing
these issues is not fun work. On the other hand, maybe direct access to
these variables from extensions is an antipattern, and we should use a
direct function call API and functions current_setting and set_config. The
overhead is usually not too big.

On Sun, Aug 22, 2021 at 02:17:16PM +0200, Pavel Stehule wrote:

Is true, so it increases differences between Windows and Unix, and fixing
these issues is not fun work. On the other hand, maybe direct access to
these variables from extensions is an antipattern, and we should use a
direct function call API and functions current_setting and set_config. The
overhead is usually not too big.

Yes, and that's what I did for one of my extensions. But that's still a bit of
overhead, and extra burden only seen when trying to have Windows compatiblity,
and I hope I can get rid of that at some point.

If direct variable access shouldn't be possible, then we should explicitly tag
those with __attribute__ ((visibility ("hidden"))) or something like that to
have a more consistent behavior.

Julien Rouhaud <rjuju123@gmail.com> writes:

On Sun, Aug 22, 2021 at 08:51:26PM +0900, Michael Paquier wrote:

... and on a parameter-basis based
on requests from extension authors. If you wish to make your
extensions able to work on Windows, that's a good idea, but I would
recommend to limit this exercise to what's really necessary for your
purpose.

I disagree. For random global variables I agree that we shouldn't mark them
all blindly, but for GUCs it's pretty clear that they're intended to be
accessible from any caller, including extensions.

Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only
of interest to particular subsystems. I do not see why being a GUC makes
something automatically more interesting than any other global variable.
Usually, the fact that one is global is only so the GUC machinery itself
can get at it, otherwise it'd be static in the owning module.

As for "extensions should be able to get at the values", the GUC machinery
already provides uniform mechanisms for doing that safely. Direct access
to the variable's internal value would be unsafe in many cases.

regards, tom lane

On Sun, Aug 22, 2021 at 09:19:42AM -0400, Tom Lane wrote:

Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only
of interest to particular subsystems. I do not see why being a GUC makes
something automatically more interesting than any other global variable.
Usually, the fact that one is global is only so the GUC machinery itself
can get at it, otherwise it'd be static in the owning module.

As for "extensions should be able to get at the values", the GUC machinery
already provides uniform mechanisms for doing that safely. Direct access
to the variable's internal value would be unsafe in many cases.

Then shouldn't we try to prevent direct access on all platforms rather than
only one?

On Sun, Aug 22, 2021 at 09:29:01PM +0800, Julien Rouhaud wrote:

On Sun, Aug 22, 2021 at 09:19:42AM -0400, Tom Lane wrote:

Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only
of interest to particular subsystems. I do not see why being a GUC makes
something automatically more interesting than any other global variable.
Usually, the fact that one is global is only so the GUC machinery itself
can get at it, otherwise it'd be static in the owning module.

As for "extensions should be able to get at the values", the GUC machinery
already provides uniform mechanisms for doing that safely. Direct access
to the variable's internal value would be unsafe in many cases.

Then shouldn't we try to prevent direct access on all platforms rather than
only one?

So since the non currently explicitly exported GUC global variables shouldn't
be accessible by third-party code, I'm attaching a POC patch that does the
opposite of v1: enforce that restriction using a new pg_attribute_hidden()
macro, defined with GCC only, to start discussing that topic.

It would probably be better to have some other macro (e.g. PG_GLOBAL_PUBLIC and
PG_GLOBAL_PRIVATE or similar) to make declarations more consistent, but given
the amount of changes it would represent I prefer to have some feedback before
spending time on that.

On Sun, Aug 22, 2021 at 09:29:01PM +0800, Julien Rouhaud wrote:

On Sun, Aug 22, 2021 at 09:19:42AM -0400, Tom Lane wrote:

Uh, no, it's exactly *not* clear. There are a lot of GUCs that are only
of interest to particular subsystems. I do not see why being a GUC makes
something automatically more interesting than any other global variable.
Usually, the fact that one is global is only so the GUC machinery itself
can get at it, otherwise it'd be static in the owning module.

As for "extensions should be able to get at the values", the GUC machinery
already provides uniform mechanisms for doing that safely. Direct access
to the variable's internal value would be unsafe in many cases.

Then shouldn't we try to prevent direct access on all platforms rather than
only one?

Agreed. If Julian says 99% of the non-export problems are GUCs, and we
can just export them all, why not do it? We already export every global
variable on Unix-like systems, and we have seen no downsides.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Bruce Momjian <bruce@momjian.us> writes:

On Sun, Aug 22, 2021 at 09:29:01PM +0800, Julien Rouhaud wrote:

Then shouldn't we try to prevent direct access on all platforms rather than
only one?

Agreed. If Julian says 99% of the non-export problems are GUCs, and we
can just export them all, why not do it? We already export every global
variable on Unix-like systems, and we have seen no downsides.

By that argument, *every* globally-visible variable should be marked
PGDLLIMPORT. But the mere fact that two backend .c files need to access
some variable doesn't mean that we want any random bit of code doing so.

And yes, I absolutely would prohibit extensions from accessing many
of them, if there were a reasonable way to do it. It would be a good
start towards establishing a defined API for extensions.

regards, tom lane

On Mon, Aug 23, 2021 at 10:15:04AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Sun, Aug 22, 2021 at 09:29:01PM +0800, Julien Rouhaud wrote:

Then shouldn't we try to prevent direct access on all platforms rather than
only one?

Agreed. If Julian says 99% of the non-export problems are GUCs, and we
can just export them all, why not do it? We already export every global
variable on Unix-like systems, and we have seen no downsides.

By that argument, *every* globally-visible variable should be marked
PGDLLIMPORT. But the mere fact that two backend .c files need to access

No, Julien says 99% need only the GUCs, so that is not the argument I am
making.

some variable doesn't mean that we want any random bit of code doing so.

And yes, I absolutely would prohibit extensions from accessing many
of them, if there were a reasonable way to do it. It would be a good
start towards establishing a defined API for extensions.

Well, if Unix needed it, we would have addressed this more regularly,
but since it is only Windows that has this _feature_, it is the rare
Windows cases that need adjustment, and usually as an afterthought since
Windows isn't usually a primary tested platform.

What I am saying is that if we blocked global variable access on Unix,
initial testing would have showed what needs to be exported and it would
not be as big a problem.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Bruce Momjian <bruce@momjian.us> writes:

On Mon, Aug 23, 2021 at 10:15:04AM -0400, Tom Lane wrote:

By that argument, *every* globally-visible variable should be marked
PGDLLIMPORT. But the mere fact that two backend .c files need to access

No, Julien says 99% need only the GUCs, so that is not the argument I am
making.

That's a claim unbacked by any evidence that I've seen. More to
the point, we already have a mechanism that extensions can/should
use to read and write GUC settings, and it's not direct access.

regards, tom lane

On Mon, Aug 23, 2021 at 10:22:51AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Mon, Aug 23, 2021 at 10:15:04AM -0400, Tom Lane wrote:

By that argument, *every* globally-visible variable should be marked
PGDLLIMPORT. But the mere fact that two backend .c files need to access

No, Julien says 99% need only the GUCs, so that is not the argument I am
making.

That's a claim unbacked by any evidence that I've seen. More to
the point, we already have a mechanism that extensions can/should
use to read and write GUC settings, and it's not direct access.

So the problem is that extensions only _need_ to use that API on
Windows, so many initially don't, or that the API is too limited?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Mon, Aug 23, 2021 at 10:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Mon, Aug 23, 2021 at 10:15:04AM -0400, Tom Lane wrote:

By that argument, *every* globally-visible variable should be marked
PGDLLIMPORT. But the mere fact that two backend .c files need to access

No, Julien says 99% need only the GUCs, so that is not the argument I am
making.

That's a claim unbacked by any evidence that I've seen. More to
the point, we already have a mechanism that extensions can/should
use to read and write GUC settings, and it's not direct access.

I clearly didn't try all single extension available out there. It's
excessively annoying to compile extensions on Windows, and also I
don't have a lot of dependencies installed so there are some that I
wasn't able to test since I'm lacking some other lib and given my
absolute lack of knowledge of that platform I didn't spent time trying
to install those.

I think I tested a dozen of "small" extensions (I'm assuming that the
big one like postgis would require too much effort to build, and they
probably already take care of Windows compatibility), and I only faced
this problem. That's maybe not a representative set, but I also doubt
that I was unlucky enough to find the few only exceptions.

On Mon, Aug 23, 2021 at 10:36 PM Bruce Momjian <bruce@momjian.us> wrote:

So the problem is that extensions only _need_ to use that API on
Windows, so many initially don't, or that the API is too limited?

The inconvenience with that API is that it's only returning c strings,
so you gave to convert it back to the original datatype. That's
probably why most of the extensions simply read from the original
exposed variable rather than using the API, because they're usually
written on Linux or similar, not because they want to mess up the
stored value.

On Mon, Aug 23, 2021 at 10:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

And yes, I absolutely would prohibit extensions from accessing many
of them, if there were a reasonable way to do it. It would be a good
start towards establishing a defined API for extensions.

The v2 patch I sent does that, at least when compiling with GCC. I
didn't find something similar for clang, but I only checked quickly.

I'm assuming that the unreasonable part is having to add some extra
attribute to the variable? Would it be acceptable if wrapped into
some other macro, as I proposed?

On Mon, Aug 23, 2021 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

And yes, I absolutely would prohibit extensions from accessing many
of them, if there were a reasonable way to do it. It would be a good
start towards establishing a defined API for extensions.

Mostly, it would make extension development more difficult for no
discernable benefit to the project.

You've made this argument many times over the years ... but we're no
closer to having an extension API than we ever were, and we continue
to get complaints about breaking stuff on Windows on a pretty regular
basis.

Honestly, it seems unimaginable that an API is ever really going to be
possible. It would be a ton of work to maintain, and we'd just end up
breaking it every time we discover that there's a new feature we want
to implement which doesn't fit into the defined API now. That's what
we do *now* with functions that third-party extensions actually call,
and with variables that they access, and it's not something that, in
my experience, is any great problem in maintaining an extension.
You're running code that is running inside somebody else's executable
and sometimes you have to adjust it for upstream changes. That's life,
and I don't think that complaints about that topic are nearly as
frequent as complaints about extensions breaking on Windows because of
missing PGDLLIMPORT markings.

And more than that, I'm pretty sure that you've previously taken the
view that we shouldn't document all the hook functions that only exist
in the backend for the purpose of extension use. I think you would
argue against a patch to go and document all the variables that are
marked PGDLLIMPORT now. So it seems to me that you're for an API when
it means that we don't have to change anything, and against an API
when it means that we don't have to change anything, which doesn't
really seem like a consistent position. I think we should be
responding to the real, expressed needs of extension developers, and
the lack of PGDLLIMPORT markings on various global variables is surely
top of the list.

It's also a bit unfair to say, well we have APIs for accessing GUC
values. It's true that we do. But if the GUC variable is, say, a
Boolean, you do not want your extension to call some function that
does a bunch of shenanigans and returns a string so that you can then
turn around and parse the string to recover the Boolean value. Even
moreso if the value is an integer or a comma-separated list. You want
to access the value as the system represents it internally, not
duplicate the parsing logic in a way that is inefficient and
bug-prone.

In short, +1 from me for the original proposal of marking all GUCs as
PGDLLIMPORT. And, heck, +1 for marking all the other global variables
that way, too. We're not solving any problem here. We're just annoying
people, mostly people who are loyal community members and steady
contributors to the project.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 08/23/21 10:36, Bruce Momjian wrote:

So the problem is that extensions only _need_ to use that API on
Windows, so many initially don't, or that the API is too limited?

I think there can be cases where it's too limited, such as when significant
computation or validation is needed between the form of the setting known
to the GUC machinery and the form that would otherwise be available in
the global.

I'm thinking, for instance, of the old example before session_timezone
was PGDLLIMPORTed, and you'd have to GETCONFIGOPTION("timezone") and
repeat the work done by pg_tzset to validate and map the timezone name
through the timezone database, to reconstruct the value that was
otherwise already available in session_timezone.

Maybe those cases aren't very numerous ... and maybe they're distinctive
enough to recognize when creating one ("hmm, I am creating a check or
assign hook that does significant work here, will it be worth exposing
a getter API for the product of the work?").

Regards,
-Chap

On 2021-Aug-23, Robert Haas wrote:

It's also a bit unfair to say, well we have APIs for accessing GUC
values. It's true that we do. But if the GUC variable is, say, a
Boolean, you do not want your extension to call some function that
does a bunch of shenanigans and returns a string so that you can then
turn around and parse the string to recover the Boolean value. Even
moreso if the value is an integer or a comma-separated list. You want
to access the value as the system represents it internally, not
duplicate the parsing logic in a way that is inefficient and
bug-prone.

In that case, why not improve the API with functions that return the
values in some native datatype? For scalars with native C types (int,
floats, Boolean etc) this is easy enough; I bet it'll solve 99% of the
problems or more.

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/

On 08/23/21 10:57, Chapman Flack wrote:

Maybe those cases aren't very numerous ... and maybe they're distinctive
enough to recognize when creating one ("hmm, I am creating a check or
assign hook that does significant work here, will it be worth exposing
a getter API for the product of the work?").

How about a generic GetTypedConfigValue(char const *name, void *into) ?

By default the type of *into would correspond to the type of the GUC
(int for an enum) and would be returned directly from *conf->variable
by a getter hook installed by default when the GUC is defined. But the
definition could also supply a getter hook that would store a value of
a different type, or computed a different way, for a GUC where that's
appropriate.

The function return could be boolean, true if such a variable exists
and isn't a placeholder.

Pro: provides read access to the typed value of any GUC, without exposing
it to overwriting. Con: has to bsearch the GUCs every time the value
is wanted.

What I'd really like:

ObserveTypedConfigValue(char const *name, void *into, int *flags, int flag)

This would register an observer of the named GUC: whenever its typed value
changes, it is written at *into and flag is ORed into *flags.

This is more restrictive than allowing arbitrary code to be hooked into
GUC changes (as changes can happen at delicate times such as error
recovery), but allows an extension to always have the current typed
value available and to cheaply know when it has changed. Observers would
be updated in the normal course of processing a GUC change, eliminating
the bsearch-per-lookup cost of the first approach.

Thinkable?

Regards,
-Chap