Re: fix more casting away of qualifiers

Hi,

Tom, Heikki, CCing you because of the part at the end of the email.

On 2026-10-07 19:58:35 +0200, Peter Eisentraut wrote:
> On 01.10.26 22:39, Andres Freund wrote:
> > ISTM just about all these volatiles are just pointless magic-wand
> > volatiles. volatile doesn't fix memory ordering (except on msvc), assigning
> > volatile to entire struct is bogus hokus pokus.
> >
> > The fact that we have to unvolatize them shows that we are fundamentally not
> > relying on the guarantees of volatile, the invoked functions don't know about
> > the volatile!
>
> I think arguments can be made that some of these volatile qualification are
> unnecessary. But for example for signal handling, using volatile
> sig_atomic_t is kind of idiomatic, and for the pgstat stuff, a change would
> probably call for a broad cleanup of volatile in that module. That might be
> too large of a detour?

volatile for enforcing immediately read from/write to memory is fine. I mean
it'll sometimes generate inefficient code and such, but it's not buggy. And
for a lot of things it's necessary. Volatile for memory accessed *across*
processes is almost always a bad smell (with some exception for low-level
primitives and reads where one can be arbitrarily out of date.

I'm too far behind on email :(

> Attached is a version that rewrites the code lightly to avoid the
> "unvolatize" casts honestly but keeps the qualifiers themselves.

WFM, you're not responsible for cleaning up everything :)

The commit you actually made had this hunk:

diff --git a/src/backend/storage/ipc/pmsignal.c b/src/backend/storage/ipc/pmsignal.c
index bdad5fdd043..08d382d73ab 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -73,16 +73,16 @@
struct PMSignalData
{
/* per-reason flags for signaling the postmaster */
- sig_atomic_t PMSignalFlags[NUM_PMSIGNALS];
+ volatile sig_atomic_t PMSignalFlags[NUM_PMSIGNALS];
/* global flags for signals from postmaster to children */
- QuitSignalReason sigquit_reason; /* why SIGQUIT was sent */
+ volatile QuitSignalReason sigquit_reason; /* why SIGQUIT was sent */
/* per-child-process flags */
int num_child_flags; /* # of entries in PMChildFlags[] */
- sig_atomic_t PMChildFlags[FLEXIBLE_ARRAY_MEMBER];
+ volatile sig_atomic_t PMChildFlags[FLEXIBLE_ARRAY_MEMBER];
};

/* PMSignalState pointer is valid in both postmaster and child processes */
-NON_EXEC_STATIC volatile PMSignalData *PMSignalState = NULL;
+NON_EXEC_STATIC PMSignalData *PMSignalState = NULL;

static void PMSignalShmemRequest(void *);
static void PMSignalShmemInit(void *);

I don't think the volatiles moved into PMSignalData are particularly
useful. They're precisely one of the cases where volatile is just pretending
to do things correctly.

I think the code was and is not really correct. At least the following
function seem to miss memory ordering:

- SetQuitSignalReason(), GetQuitSignalReason() - should have pairing memory
barriers

It *probably* is practically fine, because there will be signals around it,
and the consequences of it being wrong are not too severe (missing log
messages), but ...

- RegisterPostmasterChildActive(), MarkPostmasterChildWalSender(),
MarkPostmasterChildInactive() - because they write to PMChildFlags[], which
is also accessed by postmaster, e.g. via IsPostmasterChildWalSender(), they
really ought to use memory barriers.

The only reason this is perhaps practially OK is that functions like
SendPostmasterSignal() use kill(), which likely has the effect of a full
memory barrier.

But it seems entirely possible that postmaster sees the
PMSIGNAL_ADVANCE_STATE_MACHINE without seeing the walsender as active in the
InitWalSender() path, if there's something else waking postmaster up.

- SendPostmasterSignal(), CheckPostmasterSignal() - should have pairing memory
barriers

Tom, is there any reason this doesn't use barriers? I assume it's just
because the code is "too old"?

Greetings,

Andres