Re: consider -Wmissing-variable-declarations - Mailing list pgsql-hackers

On 10.05.24 11:53, Heikki Linnakangas wrote:
> On 09/05/2024 12:23, Peter Eisentraut wrote:
>> In [0] I had noticed that we have no automated verification that global
>> variables are declared in header files.  (For global functions, we have
>> this through -Wmissing-prototypes.)  As I mentioned there, I discovered
>> the Clang compiler option -Wmissing-variable-declarations, which does
>> exactly that.  Clang has supported this for quite some time, and GCC 14,
>> which was released a few days ago, now also supports it.  I went and
>> installed this option into the standard build flags and cleaned up the
>> warnings it found, which revealed a number of interesting things.
> 
> Nice! More checks like this is good in general.
> 
>> Attached are patches organized by sub-topic.  The most dubious stuff is
>> in patches 0006 and 0007.  A bunch of GUC-related variables are not in
>> header files but are pulled in via ad-hoc extern declarations.  I can't
>> recognize an intentional scheme there, probably just done for
>> convenience or copied from previous practice.  These should be organized
>> into appropriate header files.
> 
> +1 for moving all these to header files. Also all the "other stuff" in 
> patch 0007.

I have found a partial explanation for the "other stuff".  We have in 
launch_backend.c:

/*
  * The following need to be available to the save/restore_backend_variables
  * functions.  They are marked NON_EXEC_STATIC in their home modules.
  */
extern slock_t *ShmemLock;
extern slock_t *ProcStructLock;
extern PGPROC *AuxiliaryProcs;
extern PMSignalData *PMSignalState;
extern pg_time_t first_syslogger_file_time;
extern struct bkend *ShmemBackendArray;
extern bool redirection_done;

So these are notionally static variables that had to be sneakily 
exported for the purposes of EXEC_BACKEND.

(This probably also means my current patch set won't work cleanly on 
EXEC_BACKEND builds.  I'll need to check that further.)

However, it turns out that that comment is not completely true. 
ShmemLock, ShmemBackendArray, and redirection_done are not in fact 
NON_EXEC_STATIC.  I think they probably once were, but then they were 
needed elsewhere and people thought, if launch_backend.c (formerly 
postmaster.c) gets at them via its own extern declaration, then I will 
do that too.

ShmemLock has been like that for a longer time, but ShmemBackendArray 
and redirection_done are new like that in PG17, probably from all the 
postmaster.c refactoring.

ShmemLock and redirection_done have now escaped for wider use and should 
be in header files, as my patches are proposing.

ShmemBackendArray only exists if EXEC_BACKEND, so it's fine, but the 
comment is slightly misleading.  Maybe sticking a NON_EXEC_STATIC onto 
ShmemBackendArray, even though it's useless, would make this more 
consistent.