| From: | Rui Zhao <zhaorui126(at)gmail(dot)com> |
|---|---|
| To: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Cc: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: fix more casting away of qualifiers |
| Date: | 2026-10-09 17:32:30 |
| Message-ID: | CAHWVJhGRYSYGE+an_cZuj+de0DNZCbDFpNFRp6kN2iE7OHfjuw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Manu | 2026-10-09 17:37:50 | Re: [PG19] Wrong results from Memoize with a nondeterministic collation |
| Previous Message | Andres Freund | 2026-10-09 16:07:01 | Re: Make memory checking / sanitizing infrastructure better |