On 04.08.26 16:32, Andres Freund wrote:
Maybe this is okay, but there are a bunch more places (not touched by your
patches) that mix unsigned atomics operations with actually signed values.
Stuff like PIDs and proc numbers. I think for better overall hygiene and to
simplify broader adoption, perhaps we should introduce support for signed
atomic variables.
I'm quite hesitant to do that, at least without a lot more clear cut examples
where it actually would make the code better. I think it's rarely a good idea
to use signed variables for atomics, because you get undefined behaviour on
overflow, there's problems with bit masking, etc. IME most data in atomically
modified should actually be unsigned and probably should have been unsigned
before the conversion to atomics.
Yeah, using all unsigned would be cleaner.
I wonder what to do about this kind of suspicious-looking code that
mixes unsigned and signed:
Assert(pg_atomic_read_u32(&proc->clogGroupNext) == INVALID_PROC_NUMBER);
where
#define INVALID_PROC_NUMBER (-1)
and similarly this kind of thing
if (pg_atomic_read_u32(&slot->pss_pid) == pid)
(where pid is either pid_t or int).
We could make ProcNumber typedef'ed as unsigned instead and make
INVALID_PROC_NUMBER be UINT_MAX. That's what it effectively does now,
but that way it would be less mysterious.
(I suppose the PID stuff might go away/change significantly eventually
as part of thread stuff, but we'd probably still want an invalid/not-set
value.)
