Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve documentation for pg_atomic_unlocked_write_u32()

  2. Add pg_atomic_unlocked_write_u64

  1. pgsql: Add pg_atomic_unlocked_write_u64

    Andres Freund <andres@anarazel.de> — 2025-12-03T23:40:47Z

    Add pg_atomic_unlocked_write_u64

The 64bit equivalent of pg_atomic_unlocked_write_u32(), to be used in an
upcoming patch converting BufferDesc.state into a 64bit atomic.

Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/7902a47c20b1d5c0b7d20071f9ada6a0185bf39b

Modified Files
--------------
src/include/port/atomics.h         | 10 ++++++++++
src/include/port/atomics/generic.h |  9 +++++++++
2 files changed, 19 insertions(+)

  2. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-04T15:51:10Z

    On Wed, Dec 03, 2025 at 11:40:47PM +0000, Andres Freund wrote:
> Add pg_atomic_unlocked_write_u64
> 
> The 64bit equivalent of pg_atomic_unlocked_write_u32(), to be used in an
> upcoming patch converting BufferDesc.state into a 64bit atomic.

I noticed that this new function was defined as

	ptr->value = val;

and couldn't figure out why that was safe.  Above
pg_atomic_unlocked_write_u32(), I see this comment:

 * The write is guaranteed to succeed as a whole, i.e. it's not possible to
 * observe a partial write for any reader.  ...

But the new 64-bit version doesn't seem to be surrounded by a check for
PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY, and I found no discussion about it in
the commit message or the linked thread.  Am I missing something here?

-- 
nathan

  3. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Andres Freund <andres@anarazel.de> — 2025-12-04T15:56:12Z

    Hi,

On 2025-12-04 09:51:10 -0600, Nathan Bossart wrote:
> On Wed, Dec 03, 2025 at 11:40:47PM +0000, Andres Freund wrote:
> > Add pg_atomic_unlocked_write_u64
> > 
> > The 64bit equivalent of pg_atomic_unlocked_write_u32(), to be used in an
> > upcoming patch converting BufferDesc.state into a 64bit atomic.
> 
> I noticed that this new function was defined as
> 
> 	ptr->value = val;
> 
> and couldn't figure out why that was safe.  Above
> pg_atomic_unlocked_write_u32(), I see this comment:
> 
>  * The write is guaranteed to succeed as a whole, i.e. it's not possible to
>  * observe a partial write for any reader.  ...
> 
> But the new 64-bit version doesn't seem to be surrounded by a check for
> PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY, and I found no discussion about it in
> the commit message or the linked thread.  Am I missing something here?

The whole point of the _unlocked_ function is to use it for modifying an
atomic that doesn't need to actually be atomic when modified by that
function. The current use-case for it is to to modify BufferDesc->state for
temporary table buffers. Those obviously can't be shared across processes and
therefore don't need an atomic operation to be modified. In the referenced
thread I'm working on converting BufferDesc->state to be a 64bit atomic, hence
the need for pg_atomic_unlocked_write_u64().

I didn't notice that the comment for pg_atomic_unlocked_write_u32() makes that
claim about partial writes not being visible. I think we should just remove
that claim.

Greetings,

Andres Freund

  4. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-04T16:03:22Z

    On Thu, Dec 04, 2025 at 10:56:12AM -0500, Andres Freund wrote:
> The whole point of the _unlocked_ function is to use it for modifying an
> atomic that doesn't need to actually be atomic when modified by that
> function. The current use-case for it is to to modify BufferDesc->state for
> temporary table buffers. Those obviously can't be shared across processes and
> therefore don't need an atomic operation to be modified. In the referenced
> thread I'm working on converting BufferDesc->state to be a 64bit atomic, hence
> the need for pg_atomic_unlocked_write_u64().
> 
> I didn't notice that the comment for pg_atomic_unlocked_write_u32() makes that
> claim about partial writes not being visible. I think we should just remove
> that claim.

+1 to updating the comment with this context.

-- 
nathan

  5. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Andres Freund <andres@anarazel.de> — 2025-12-04T17:30:28Z

    Hi,

On 2025-12-04 10:03:22 -0600, Nathan Bossart wrote:
> On Thu, Dec 04, 2025 at 10:56:12AM -0500, Andres Freund wrote:
> > The whole point of the _unlocked_ function is to use it for modifying an
> > atomic that doesn't need to actually be atomic when modified by that
> > function. The current use-case for it is to to modify BufferDesc->state for
> > temporary table buffers. Those obviously can't be shared across processes and
> > therefore don't need an atomic operation to be modified. In the referenced
> > thread I'm working on converting BufferDesc->state to be a 64bit atomic, hence
> > the need for pg_atomic_unlocked_write_u64().
> > 
> > I didn't notice that the comment for pg_atomic_unlocked_write_u32() makes that
> > claim about partial writes not being visible. I think we should just remove
> > that claim.
> 
> +1 to updating the comment with this context.

Hm. Aside from the above issue, the reference to atomics emulation in the
comment is also obsolete since 81385261362.

How about:

/*
 * pg_atomic_unlocked_write_u32 - unlocked write to atomic variable.
 *
 * Write to an atomic variable, without atomicity guarantees. I.e. it is not
 * guaranteed that a concurent reader will not see a torn value, nor to
 * guaranteed to correctly interact with concurrent read-modify-write
 * operations like pg_atomic_compare_exchange_u32.  This should only be used
 * in cases where minor performance regressions due to atomic operations are
 * unacceptable and where exclusive access is guaranteed due to some external
 * means.
 *
 * No barrier semantics.
 */


We could actually guarantee, in the 32bit case, that a concurrent reader would
not see a torn value, but ISTM that any such user should not use _unlocked_,
and this way we don't need separate documentation for the 64bit case.

Greetings,

Andres Freund

  6. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-04T18:15:55Z

    Andres Freund <andres@anarazel.de> writes:
> How about:

> /*
>  * pg_atomic_unlocked_write_u32 - unlocked write to atomic variable.
>  *
>  * Write to an atomic variable, without atomicity guarantees. I.e. it is not
>  * guaranteed that a concurent reader will not see a torn value, nor to

grammar police: s/nor to/nor is this/

>  * guaranteed to correctly interact with concurrent read-modify-write
>  * operations like pg_atomic_compare_exchange_u32.  This should only be used
>  * in cases where minor performance regressions due to atomic operations are
>  * unacceptable and where exclusive access is guaranteed due to some external
>  * means.
>  *
>  * No barrier semantics.
>  */

LGTM otherwise.

			regards, tom lane

  7. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Nathan Bossart <nathandbossart@gmail.com> — 2025-12-04T18:41:26Z

    On Thu, Dec 04, 2025 at 01:15:55PM -0500, Tom Lane wrote:
> LGTM otherwise.

+1

-- 
nathan

  8. Re: pgsql: Add pg_atomic_unlocked_write_u64

    Andres Freund <andres@anarazel.de> — 2025-12-09T04:12:34Z

    Hi,

On 2025-12-04 12:41:26 -0600, Nathan Bossart wrote:
> On Thu, Dec 04, 2025 at 01:15:55PM -0500, Tom Lane wrote:
> > LGTM otherwise.
> 
> +1

Thanks for the reviews Tom and Nathan. Pushed with minor further tweaks
(s/due to/via/, s/concurent/concurrent/).

Greetings,

Andres Freund