fix more casting away of qualifiers

On 18/08/2026 12:04, Peter Eisentraut wrote:

The attached patches fix more cases where qualifiers (const, volatile)
are cast away either accidentally, or unnecessarily, or where it can be
worked around easily.

I split these into tiny bits to simplify review and to show that they
are all independent.  But they could perhaps be committed all together.
(See also similar commit 3f988629805.)

Thanks for the cleanups!

diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index e2c2919484e..defaa796a0c 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -971,6 +971,7 @@ show_all_settings(PG_FUNCTION_ARGS)
while (call_cntr < max_calls)	/* do when there is more left to send */
{
struct config_generic *conf = guc_vars[call_cntr];
+		const char *cvalues[NUM_PG_SETTINGS_ATTS];
char	   *values[NUM_PG_SETTINGS_ATTS];
HeapTuple	tuple;
Datum		result;
@@ -984,7 +985,14 @@ show_all_settings(PG_FUNCTION_ARGS)
}

/* extract values for the current variable */
-		GetConfigOptionValues(conf, (const char **) values);
+		GetConfigOptionValues(conf, cvalues);
+
+		/*
+		 * This is so that both GetConfigOptionValues() and
+		 * BuildTupleFromCStrings() are satisfied about the const-ness without
+		 * triggering warnings.
+		 */
+		memcpy(values, cvalues, sizeof(cvalues));

/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);

This seems hacky. Can we change BuildTupleFromCStrings() to take a const
instead? Maybe that was part of your "difficult half" already?

I guess that requires changing all the callers: You get a warning if
pass a "char **" to a "const char **". Is there a way with some macro
magic or something that you could accept both?

All else look good to me at a quick glance.

- Heikki

On 18.08.26 12:00, Heikki Linnakangas wrote:

This seems hacky. Can we change BuildTupleFromCStrings() to take a const
instead? Maybe that was part of your "difficult half" already?

I guess that requires changing all the callers: You get a warning if
pass a "char **" to a "const char **". Is there a way with some macro
magic or something that you could accept both?

All else look good to me at a quick glance.

I have committed these patches except the one above. I have an idea for
that, but I'll prepare that separately.

On 25 Aug 2026, at 10:42, Peter Eisentraut <peter@eisentraut.org> wrote:

I have committed these patches except the one above.

BF member sifaka complains due to (it seems like) macOS bundling readline 3.0
in the base OS. Maybe we need a configure/meson check for this?

tab-complete.in.c:1525:19: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
1525 | rl_readline_name = pset.progname;
| ^ ~~~~~~~~~~~~~
1 error generated.

--
Daniel Gustafsson

On 25.08.26 10:52, Daniel Gustafsson wrote:

On 25 Aug 2026, at 10:42, Peter Eisentraut <peter@eisentraut.org> wrote:

I have committed these patches except the one above.

BF member sifaka complains due to (it seems like) macOS bundling readline 3.0
in the base OS. Maybe we need a configure/meson check for this?

tab-complete.in.c:1525:19: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
1525 | rl_readline_name = pset.progname;
| ^ ~~~~~~~~~~~~~
1 error generated.

Ok, thanks, I'm going to revert it. I was anticipating something like
this, but at least now we know for sure.

On 18.08.26 11:04, Peter Eisentraut wrote:

The attached patches fix more cases where qualifiers (const, volatile)
are cast away either accidentally, or unnecessarily, or where it can be
worked around easily.

I split these into tiny bits to simplify review and to show that they
are all independent.  But they could perhaps be committed all together.
(See also similar commit 3f988629805.)

I have a local WIP branch that fixes all remaining -Wcast-qual warnings.
 The attached patches are the "easy" half of that.  I plan to propose
addressing the other half separately later.

Here are more patches for this. (I suppose this is half of the
above-mentioned other half.)

The first two patches address the issue that unconstify cannot be used
for global variables. I'm introducing an unconstify_constexpr that can
be used for that but only works for GCC.

The third patch adjusts unconstify, unconstify_constexpr, and unvolatize
so that they don't trigger -Wcast-qual warnings. This will be useful
later when we have fixed all the remaining issues and we can turn on
-Wcast-qual.

Patches 4 through 7 add various unconstify and unvolatize in place of or
in addition to existing casts.

The last patch would then turn on the warning option, but it's only for
illustration right, as there is more (increasingly complicated) stuff to
fix after this.

Hi Peter,

I like this cleanup, especially removing the need to cast const char *
to char * just to call a function that only reads the input.

The first two patches address the issue that unconstify cannot be used
for global variables.

With GCC 10.2 and Clang 15, I checked that unconstify_constexpr works
in static initializers and that both unconstify_constexpr and
unconstify still reject casts to an incompatible pointer type.
V2 0001-0007 compiled with both compilers. I enabled OpenSSL and
GSSAPI in both builds to cover their API casts in 0004.

I also ran the same tests with both builds to check for behavior
changes: core regression, postgres_fdw, psql and PL/Python. All passed.

One small thing in 0005, in src/backend/utils/adt/float.c: the comment
above float8in_internal() still says:

"num" could validly be declared "const char *", but that results in an
unreasonable amount of extra casting both here and in callers,
so we don't.

Could we remove those two lines now that num is const char *?

Otherwise, 0001-0007 look good to me.

Regards,
Rui

Hi,

On 2026-09-15 10:03:57 +0200, Peter Eisentraut wrote:

From d5384f9e391a10177fd7bf06c866e62834f52896 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 15 Sep 2026 08:43:09 +0200
Subject: [PATCH v2 5/8] Change float{4,8}in_internal take const char * input
argument

This makes the function signature match strtof()/strtod().

float4
-float4in_internal(char *num, char **endptr_p,
+float4in_internal(const char *num, char **endptr_p,
const char *type_name, const char *orig_string,
struct Node *escontext)
{

it's somewhat gross to have an API that has char **endptr_p point into a const
char *...

@@ -269,37 +269,37 @@ float4in_internal(char *num, char **endptr_p,
if (pg_strncasecmp(num, "NaN", 3) == 0)
{
val = get_float4_nan();
-			endptr = num + 3;
+			endptr = unconstify(char *, num) + 3;
}
else if (pg_strncasecmp(num, "Infinity", 8) == 0)
{
val = get_float4_infinity();
-			endptr = num + 8;
+			endptr = unconstify(char *, num) + 8;
}
...

I don't like that this is adding a large number of unconstify()s. The only
thing that makes it a bit awkward to make endptr const itself is
endptr_p. It'd be tempting to just remove it, most callers don't even use it -
but unfortunately it looks like single_decode() does.

It seems like it'd be less ugly to just unconstify the two places that
actually need it (the call to strtod() and the assignment to *endptr_p).

From bb7d887646ec7f1a83d1a5f9e370861a82c86ff8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 15 Sep 2026 08:43:09 +0200
Subject: [PATCH v2 7/8] Additional unvolatize uses

Add some uses of unvolatize to silence warnings that would be
triggered by -Wcast-qual.

Note that the MemSet() calls would be normal "discards qualifier"
warnings if memset() (or another function with a prototype, not a
macro) were used.

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!

Greetings,

Andres Freund

On 01.10.26 18:44, Rui Zhao wrote:

One small thing in 0005, in src/backend/utils/adt/float.c: the comment
above float8in_internal() still says:

"num" could validly be declared "const char *", but that results in an
unreasonable amount of extra casting both here and in callers,
so we don't.

Could we remove those two lines now that num is const char *?

I have committed a few of these patches that did not receive any other
comments, and I will incorporate your feedback into the updated patch
series for the rest. Thanks.

On 01.10.26 22:39, Andres Freund wrote:

I don't like that this is adding a large number of unconstify()s. The only
thing that makes it a bit awkward to make endptr const itself is
endptr_p. It'd be tempting to just remove it, most callers don't even use it -
but unfortunately it looks like single_decode() does.

It seems like it'd be less ugly to just unconstify the two places that
actually need it (the call to strtod() and the assignment to *endptr_p).

Yeah, done that way in the new version.

From bb7d887646ec7f1a83d1a5f9e370861a82c86ff8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 15 Sep 2026 08:43:09 +0200
Subject: [PATCH v2 7/8] Additional unvolatize uses

Add some uses of unvolatize to silence warnings that would be
triggered by -Wcast-qual.

Note that the MemSet() calls would be normal "discards qualifier"
warnings if memset() (or another function with a prototype, not a
macro) were used.

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?

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

Hi Peter,

Thanks very much for continuing this cleanup. V3 0001 looks good to me.

In 0002, could the inner loop in ProcSignalShmemInit() use j instead
of i? It shadows the outer loop's i, and my GCC 10.2 build reports
"declaration of 'i' shadows a previous local" with the default
-Wshadow=local option. I suggest:

for (int j = 0; j < NUM_PROCSIGNALS; j++)
slot->pss_signalFlags[j] = false;

for the pgstat stuff, a change
would probably call for a broad cleanup of volatile in that module.

I agree with keeping this patch focused on the casts. In
pgstat_progress_start_command(), beentry is declared as:

volatile PgBackendStatus *beentry = MyBEEntry;

The new clearing loop remains between the existing update macros:

PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
beentry->st_progress_command = cmdtype;
beentry->st_progress_command_target = relid;
for (int i = 0; i < PGSTAT_NUM_PROGRESS_PARAM; i++)
beentry->st_progress_param[i] = 0;
PGSTAT_END_WRITE_ACTIVITY(beentry);

MemSet casts its destination to void *, discarding volatile. The loop
writes to the array through beentry without that cast. The BEGIN/END
macros still update the change counter and issue write barriers around
those writes, so readers still reject a partially updated entry.

This change preserves the existing synchronization. To decide whether
volatile can be removed, we would need to review its other uses in
pgstat, so I would leave that question out of this patch.

For PMSignalData, the new member declarations are:

volatile sig_atomic_t PMSignalFlags[NUM_PMSIGNALS];
volatile QuitSignalReason sigquit_reason; /* why SIGQUIT was sent */
int num_child_flags; /* # of entries in PMChildFlags[] */
volatile sig_atomic_t PMChildFlags[FLEXIBLE_ARRAY_MEMBER];

Only num_child_flags is declared without volatile. It is initialized
before postmaster children use it and does not change afterwards, so
that looks fine. The members carrying signals retain volatile.

I tested with GCC 10.2 and Clang 15 at -O2 on Linux, with EXEC_BACKEND
enabled in both builds to cover the changed PMSignalState declaration
and startup code.

Core regression and postgres_fdw passed with both builds, exercising
0001's float input changes. To check for regressions in the signal
handling code changed by 0002, I also ran the full recovery suite,
which exercises process startup, crash recovery and shutdown. It
passed with both builds.

Apart from the shadow warning, 0002 looks good to me too.

Regards,
Rui

On 09.10.26 19:32, Rui Zhao wrote:

Hi Peter,

Thanks very much for continuing this cleanup. V3 0001 looks good to me.

In 0002, could the inner loop in ProcSignalShmemInit() use j instead
of i? It shadows the outer loop's i, and my GCC 10.2 build reports
"declaration of 'i' shadows a previous local" with the default
-Wshadow=local option. I suggest:

Committed with that fix, thanks.

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

Andres Freund <andres@anarazel.de> writes:

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

Probably.

regards, tom lane