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.
