Re: Non-reproducible AIO failure - Mailing list pgsql-hackers

On 2025-06-06 15:21:13 -0400, Tom Lane wrote:

It's a mistake to think that this is a compiler bug.  The C standard
explicitly allows compilers to use word-wide operations to access
bit-field struct members.  Such accesses may fetch or overwrite other
bit-fields in the same word, using a non-atomic read/modify/write
sequence.  I don't have a recent C standard at hand, but I found this
quote on the web [1]:

The C Standard, 3.17, paragraph 3 [ISO/IEC 9899:2024], states
    Note 2 to entry: A bit-field and an adjacent non-bit-field member    are in separate memory locations. The same applies to two    bit-fields, if one is declared inside a nested structure    declaration and the other is not, or if the two are separated by a    zero-length bit-field declaration, or if they are separated by a    non-bit-field member declaration ...

So it's our code that is busted.  No doubt, what is happening is
that process A is fetching two fields, modifying one of them,
and storing the word back (with the observed value of the other
field) concurrently with some other process trying to update
the other field.  So the other process's update is lost.

There shouldn't be any concurrent accesses here, so I don't really see how the
above would explain the problem (the IO can only ever be modified by one
backend, initially the "owning backend", then, when submitted, by the IO
worker, and then again by the backend).

FWIW,  I tried the idea Matthias proposed upthread:
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -98,16 +98,17 @@ struct PgAioHandle
        /* all state updates should go through pgaio_io_update_state() */
        PgAioHandleState state:8;
 
+       /* bitfield of PgAioHandleFlags */
+       uint8           flags;
+
        /* what are we operating on */
        PgAioTargetID target:8;
 
+       uint8           num_callbacks;
+
        /* which IO operation */
        PgAioOp         op:8;
 
-       /* bitfield of PgAioHandleFlags */
-       uint8           flags;
-
-       uint8           num_callbacks;
 
        /* using the proper type here would use more space */
        uint8           callbacks[PGAIO_HANDLE_MAX_CALLBACKS];

and got 120 iterations passed cleanly. Without this change, I got failures
on iterations 41, 5, 20, Not a proof, just an observation...

May be the op field gets broken because of a (concurrent?) write to
not that field, by to state or target in some other place?

I also tried to reproduce the same on an ARM server again with clang 18,
using -O2 and -O3, and we with Konstantin didn't see such a sophisticated
assembly for pgaio_io_stage() — the code is much simpler and the failure
is not reproduced. The triplet there is:
PostgreSQL 18beta1 on aarch64-unknown-linux-gnu, compiled by Ubuntu clang version 18.1.3 (1ubuntu1), 64-bit

On that my MacBook, the triplet is:
PostgreSQL 18beta1 on aarch64-apple-darwin23.5.0, compiled by Apple clang version 16.0.0 (clang-1600.0.26.6), 64-bit
indri:
PostgreSQL 18beta1 on aarch64-apple-darwin24.5.0, compiled by Apple clang version 17.0.0 (clang-1700.0.13.3), 64-bit
sifaka:
PostgreSQL 18devel on aarch64-apple-darwin24.4.0, compiled by Apple clang version 17.0.0 (clang-1700.0.13.3), 64-bit

So it looks like "aarch64-apple" enables some extra optimizations with
bitfields...

By the way, there are also some other places with adjacent bitfields, e, g.:
typedef union PgAioTargetData
{
    struct
    {
        RelFileLocator rlocator;    /* physical relation identifier */
        BlockNumber blockNum;   /* blknum relative to begin of reln */
        BlockNumber nblocks;
        ForkNumber  forkNum:8;  /* don't waste 4 byte for four values */
        bool        is_temp:1;  /* proc can be inferred by owning AIO */
        bool        skip_fsync:1;
    }           smgr;

and
typedef struct PgStat_KindInfo
{
    /*
     * Do a fixed number of stats objects exist for this kind of stats (e.g.
     * bgwriter stats) or not (e.g. tables).
     */
    bool        fixed_amount:1;

    /*
     * Can stats of this kind be accessed from another database? Determines
     * whether a stats object gets included in stats snapshots.
     */
    bool        accessed_across_databases:1;

    /* Should stats be written to the on-disk stats file? */
    bool        write_to_file:1;
...

Best regards,
Alexander Lakhin