Date: Tue, 8 Sep 2026 12:00:47 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Andres Freund <andres@anarazel.de>,
	Peter Eisentraut <peter@eisentraut.org>,
	pgsql-hackers@postgresql.org
Subject: Re: convert various variables to atomics
Message-ID: <aqA_P7Uwub-MDXOO@nathan>
References: <alAJeRRzehDjLaF1@nathan>
 <9d8c317d-d933-46c7-b675-4b9308eaca2b@eisentraut.org>
 <amDAZAVob4DJjUfW@nathan>
 <ycvruij7554tlhw6w7bg4lqtibd52qguo52iojyou7ejdv5jrm@qyyr5f3hdsue>
 <amDGRZxnlmTVjkCe@nathan>
 <amJx4Lwx4nuuExYT@nathan>
 <207c0bfb-6e06-4358-bb2f-c961915efc36@eisentraut.org>
 <hi4rsxwu3ioas5rmuwfnu2gisqb2rd6g2uq56r3pvzk7clismo@oyndzrrylnir>
 <3856d1cf-53a8-414b-98d9-829d5a455a86@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3856d1cf-53a8-414b-98d9-829d5a455a86@iki.fi>
Archived-At: <https://www.postgresql.org/message-id/aqA_P7Uwub-MDXOO%40nathan>
Precedence: bulk

I committed v2-{0003,0004,0008,0009}, and I looked closer at the signed
versus unsigned mismatches and determined the following:

* v2-0001: We are changing a variable from signed to unsigned, but the code
goes out of its way to avoid negative values and signed integer overflow,
so I don't think there are any real problems here.  The only atomic
arithmetic operation is in SICleanupQueue() where we subtract
MSGNUMWRAPAROUND, which IIUC should never produce a negative value.  That
being said, I don't think it would be too disruptive to switch all relevant
variables to uint32 as a prerequisite patch.  I don't see any particular
reason for those variables to be signed, anyway.

* v2-0002: The variable in question stores a value from the
SharedBitmapState enum.  There's no atomic arithmetic involved: we just
write and compare-exchange.  At a glance, I didn't see any existing
examples of using enum values for an atomic variable, but I think it's
fine.  I believe the C standard guarantees the enum values will be 0, 1, 2,
etc., and even if we did set some enumeration constants to negative values,
it wouldn't matter because we aren't doing arithmetic with it (and are
probably unlikely to anytime soon).  So, IMHO this one is fine as-is.

* v2-0005: Since 0004 is committed, startupBufferPinWaitBuf is now a
Buffer.  Buffer is still a signed integer, but since we don't set
startupBufferPinWaitBuf to a local buffer (only to a shared buffer or
InvalidBuffer (0)), it'll always be >= 0.  Furthermore, we don't do any
sort of atomic arithmetic with this variable; it's hidden behind setter and
getter functions.  I think this one is fine.

* v2-0006: The variables in this one are only ever incremented by 1, and
they track the number of workers for a given operation, which I can't
imagine approaches anything even close to overflowing an integer.  Not to
mention that we're using signed integers for all the relevant variables
today...  I don't see any risk here, but I'll try to switch the relevant
variables to unsigned as a prerequisite and see how it looks.  If it's too
invasive, it's probably not worth worrying about.

* v2-0007: I think this one already does all the work to avoid any signed
versus unsigned mismatches.  The Assert() in SharedFileSetOnDetach() looks
bogus, though, so I'll fix that.  I guess there could be some risk of
overflow in the "refcnt + 1" in SharedFileSetAttach(), but we don't handle
that at all today, so I don't think we need to worry about it.  (In theory
this patch actually reduces the overflow risk by switching to unsigned,
anyway.)

-- 
nathan