Re: convert various variables to atomics

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.)