Re: Mark all GUC variable as PGDLLIMPORT

On Mon, May 9, 2022 at 3:15 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, May 06, 2022 at 04:49:24PM -0700, Andres Freund wrote:
> > Just noticed that
> > extern sigset_t UnBlockSig,
> >                       BlockSig,
> >                       StartupBlockSig;
> > are unadorned.
>
> mark_pgdllimport.pl is not able to capture this part of the change
> because of this logic, where we assume that the header line has to
> finish with a semicolon:
> # Variable declarations should end in a semicolon. If we see an
> # opening parenthesis, it's probably a function declaration.
> $needs_pgdllimport = 0 if $stripped_line !~ /;$/ ||
>     $stripped_line =~ /\(/;
>
> It is quite common to define one variable per line, so I would not put
> the blame on the script and just update pqsignal.h.  And it is common
> to finish lines with a comma for function declarations..

Right. I didn't try to equip the script with a real C parser. Just
enough to catch our typical style - which those declarations aren't.

> > There's also a number of variables that are only declared in .c files that
> > !windows can still access. Some likely aren't worth caring about, but some
> > others are underlying GUCs, so we probably do? E.g.
> > CommitDelay
> > CommitSiblings
> > default_tablespace
> > ignore_checksum_failure
> > ignore_invalid_pages
> > Log_disconnections
> > ssl_renegotiation_limit
> > temp_tablespaces
> > wal_level_options
>
> These are indeed declared in .c files.  So you mean that we'd better
> declare them in headers and mark them as PGDLLIMPORT?  I am not sure
> if that's worth the addition, nobody has asked for these to be
> available yet, AFAIK.

Completely agree. The agreed-upon charter was to adjust the header
files, not move any declarations into header files. I'm not against
doing that; I think it's good for extensions to have access to GUC
values. But it wasn't the mission.

> > Also noticed that the bbsink_ops variables are const, instead of static const,
> > was that intentional?
>
> Yep, that looks like an error.
>
> While on it, I think that it would be a good idea to document in the
> script that we need pass down a list of header files as arguments to
> rewrite them.

Either of you please feel free to change these things, at least as far
as I'm concerned.

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