Re: consider -Wmissing-variable-declarations

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.