Re: Mark all GUC variable as PGDLLIMPORT

Hi,

On 2022-02-14 12:45:08 -0500, Robert Haas wrote:
> On Mon, Feb 14, 2022 at 12:34 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > * If we institute a policy that all GUCs should be PGDLLIMPORT'd,
> > how will we enforce that new patches follow that? I don't want to be
> > endlessly going back and adding forgotten PGDLLIMPORT markers.
>
> It seems to me that it would be no different from bumping catversion
> or updating nodefuncs.c: yes, it will get forgotten sometimes, but
> hopefully people will mostly get used to it just like they do with
> dozens of other PG-specific coding rules. I don't see that it's likely
> to generate more than a handful of commits per year, so it doesn't
> really seem worth worrying about to me, but YMMV. Maybe it's possible
> to write a perl script to verify it, although it seems like it might
> be complicated to code that up.

I think it'd be better if we had scripts ensuring this stays sane. Several of
your examples do have that. Whereas I don't see what would cause us to missing
PGDLLIMPORTs for GUCs, given it's a windows only issue and that you need an
extension using the GUC with omitted PGDLLIMPORT.

> An alternative rule which would dodge that particular issue would be
> to just slap PGDLLIMPORT on every global variable in every header
> file. That would arguably be a simpler rule, though it means even more
> PGDLLIMPORT declarations floating around.

That would have the advantage of being comparatively easy to check in an
automated way. Might even be cheap enough to just make it part of the build.

But it seems like it'd be a fair amount of work and cause a lot of patch
rebasing pain? If we end up going that way, we should schedule this to happen
just after the feature freeze, I think.

If we consider doing this for all extern variables, we should think about
doing this for headers *and* functions. That'd allow us to get rid of the
fairly complicated logic to generate the .def file for the postgres binary on
windows (src/tools/gendef.pl). And maybe also the related thing on AIX
(src/backend/port/aix/mkldexport.sh)

I'm kind of partial to the "add explicit visibility information to everything"
approach because the windows export file generation is a decent chunk of the
meson patchset. And what's missing to make it work on AIX...

Greetings,

Andres Freund