Horton
Menu

Thread

  1. BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-25T14:51:30Z

    Good day, Andres and hackers.

UnlockBufHdrExt does:

   buf_state |= set_bits;
   buf_state &= ~unset_bits;
   buf_state &= ~BM_LOCKED;

TerminateBufferIO unconditionally does:

   unset_flag_bits |= BM_IO_ERROR;

Due to this, AbortBufferIO and buffer_readv_complete_one are failed
to set BM_IO_ERROR with call to TerminateBufferIO.

It was found with proprietary code that was triggered on
PGAIO_RS_ERROR and made assertion on BM_IO_ERROR presence.

-- 
regards
Yura Sokolov aka funny-falcon

  2. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Andres Freund <andres@anarazel.de> — 2026-03-26T20:29:23Z

    Hi,

On 2026-03-25 17:51:30 +0300, Yura Sokolov wrote:
> UnlockBufHdrExt does:
> 
>    buf_state |= set_bits;
>    buf_state &= ~unset_bits;
>    buf_state &= ~BM_LOCKED;
> 
> TerminateBufferIO unconditionally does:
> 
>    unset_flag_bits |= BM_IO_ERROR;
> 
> Due to this, AbortBufferIO and buffer_readv_complete_one are failed
> to set BM_IO_ERROR with call to TerminateBufferIO.
> 
> It was found with proprietary code that was triggered on
> PGAIO_RS_ERROR and made assertion on BM_IO_ERROR presence.

That's clearly not right.  Care to write a patch?  I think we should add a
test for this in src/test/modules/test_aio too. As we don't rely on things
like BM_IO_ERROR this is quite easy to not notice.

Greetings,

Andres Freund

  3. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-27T09:04:15Z

    26.03.2026 23:29, Andres Freund wrote:
> Hi,
> 
> On 2026-03-25 17:51:30 +0300, Yura Sokolov wrote:
>> UnlockBufHdrExt does:
>>
>>    buf_state |= set_bits;
>>    buf_state &= ~unset_bits;
>>    buf_state &= ~BM_LOCKED;
>>
>> TerminateBufferIO unconditionally does:
>>
>>    unset_flag_bits |= BM_IO_ERROR;
>>
>> Due to this, AbortBufferIO and buffer_readv_complete_one are failed
>> to set BM_IO_ERROR with call to TerminateBufferIO.
>>
>> It was found with proprietary code that was triggered on
>> PGAIO_RS_ERROR and made assertion on BM_IO_ERROR presence.
> 
> That's clearly not right.  Care to write a patch?  I think we should add a
> test for this in src/test/modules/test_aio too. As we don't rely on things
> like BM_IO_ERROR this is quite easy to not notice.
I thought, fix is too small to go the way of patches.
I don't mind if you just push it.

But if I can save your time on writing test, I'll try.

...

I believe, proper way is to add assertion in UnlockBufHdrExt:

    Assert(!(set_bits & unset_bits));

And make TerminateBufferIO to exclude BM_IO_ERROR if it is among
set_flag_bits.

Is it ok?

-- 
regards
Yura Sokolov aka funny-falcon

  4. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-27T18:30:17Z

    Good day,

27.03.2026 12:04, Yura Sokolov wrote:
> 26.03.2026 23:29, Andres Freund wrote:
>> Hi,
>>
>> On 2026-03-25 17:51:30 +0300, Yura Sokolov wrote:
>>> UnlockBufHdrExt does:
>>>
>>>    buf_state |= set_bits;
>>>    buf_state &= ~unset_bits;
>>>    buf_state &= ~BM_LOCKED;
>>>
>>> TerminateBufferIO unconditionally does:
>>>
>>>    unset_flag_bits |= BM_IO_ERROR;
>>>
>>> Due to this, AbortBufferIO and buffer_readv_complete_one are failed
>>> to set BM_IO_ERROR with call to TerminateBufferIO.
>>>
>>> It was found with proprietary code that was triggered on
>>> PGAIO_RS_ERROR and made assertion on BM_IO_ERROR presence.
>>
>> That's clearly not right.  Care to write a patch?  I think we should add a
>> test for this in src/test/modules/test_aio too. As we don't rely on things
>> like BM_IO_ERROR this is quite easy to not notice.
> I thought, fix is too small to go the way of patches.
> I don't mind if you just push it.
> 
> But if I can save your time on writing test, I'll try.
> 
> ...
> 
> I believe, proper way is to add assertion in UnlockBufHdrExt:
> 
>     Assert(!(set_bits & unset_bits));
> 
> And make TerminateBufferIO to exclude BM_IO_ERROR if it is among
> set_flag_bits.
> 
> Is it ok?

Here patchset is.

I've tried to modify 001_aio.pl to test presence BM_IO_ERROR after read
failure.

I've also added FlushBuffer failure test in second patch (v1-002) using
injection point. I don't know if 001-aio.pl is a good place for since write
is not async yet.

v1-003 just mades DebugPrintBufferRefcount prettier.

-- 
regards
Yura Sokolov aka funny-falcon

  5. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-30T08:25:55Z

    27.03.2026 21:30, Yura Sokolov wrote:
>> 26.03.2026 23:29, Andres Freund wrote:
>>>
>>> That's clearly not right.  Care to write a patch?
> 
> Here patchset is.
Good day, Andres.

Have you any recommendations for patch?

Should I create CF item for patch?

-- 
regards
Yura Sokolov aka funny-falcon

  6. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-31T09:15:59Z

    27.03.2026 21:30, Yura Sokolov пишет:
> Good day,
> 
> 27.03.2026 12:04, Yura Sokolov wrote:
>> 26.03.2026 23:29, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2026-03-25 17:51:30 +0300, Yura Sokolov wrote:
>>>> UnlockBufHdrExt does:
>>>>
>>>>    buf_state |= set_bits;
>>>>    buf_state &= ~unset_bits;
>>>>    buf_state &= ~BM_LOCKED;
>>>>
>>>> TerminateBufferIO unconditionally does:
>>>>
>>>>    unset_flag_bits |= BM_IO_ERROR;
>>>>
>>>> Due to this, AbortBufferIO and buffer_readv_complete_one are failed
>>>> to set BM_IO_ERROR with call to TerminateBufferIO.
>>>>
>>>> It was found with proprietary code that was triggered on
>>>> PGAIO_RS_ERROR and made assertion on BM_IO_ERROR presence.
>>>
>>> That's clearly not right.  Care to write a patch?  I think we should add a
>>> test for this in src/test/modules/test_aio too. As we don't rely on things
>>> like BM_IO_ERROR this is quite easy to not notice.
>> I thought, fix is too small to go the way of patches.
>> I don't mind if you just push it.
>>
>> But if I can save your time on writing test, I'll try.
>>
>> ...
>>
>> I believe, proper way is to add assertion in UnlockBufHdrExt:
>>
>>     Assert(!(set_bits & unset_bits));
>>
>> And make TerminateBufferIO to exclude BM_IO_ERROR if it is among
>> set_flag_bits.
>>
>> Is it ok?
> 
> Here patchset is.
> 
> I've tried to modify 001_aio.pl to test presence BM_IO_ERROR after read
> failure.
> 
> I've also added FlushBuffer failure test in second patch (v1-002) using
> injection point. I don't know if 001-aio.pl is a good place for since write
> is not async yet.
> 
> v1-003 just mades DebugPrintBufferRefcount prettier.

rebased.

-- 
regards
Yura Sokolov aka funny-falcon

  7. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Michael Paquier <michael@paquier.xyz> — 2026-04-06T08:21:40Z

    On Mon, Mar 30, 2026 at 11:25:55AM +0300, Yura Sokolov wrote:
> Have you any recommendations for patch?
> 
> Should I create CF item for patch?

The business with unset_flag_bits is new as of HEAD, so this could
qualify as an open item, to be tracked here:
https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
--
Michael

  8. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Melanie Plageman <melanieplageman@gmail.com> — 2026-06-02T16:15:40Z

    On Mon, Apr 6, 2026 at 4:21 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Mar 30, 2026 at 11:25:55AM +0300, Yura Sokolov wrote:
> > Have you any recommendations for patch?
> >
> > Should I create CF item for patch?
>
> The business with unset_flag_bits is new as of HEAD, so this could
> qualify as an open item, to be tracked here:
> https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

There hasn't been recent activity on this thread and it is one of the
oldest open items. Even if we don't have time to review the test right
now, it is probably worth pushing the fix. Though we have just missed
beta1...

- Melanie

  9. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Andres Freund <andres@anarazel.de> — 2026-06-03T03:21:49Z

    Hi,

On 2026-06-02 12:15:40 -0400, Melanie Plageman wrote:
> On Mon, Apr 6, 2026 at 4:21 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, Mar 30, 2026 at 11:25:55AM +0300, Yura Sokolov wrote:
> > > Have you any recommendations for patch?
> > >
> > > Should I create CF item for patch?
> >
> > The business with unset_flag_bits is new as of HEAD, so this could
> > qualify as an open item, to be tracked here:
> > https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
> 
> There hasn't been recent activity on this thread and it is one of the
> oldest open items. Even if we don't have time to review the test right
> now, it is probably worth pushing the fix. Though we have just missed
> beta1...

Ugh. I had lost track of this.  I'll get back to it as soon as I have squared
the CI stuff away.

Greetings,

Andres Freund

  10. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Andres Freund <andres@anarazel.de> — 2026-06-15T16:18:43Z

    Hi,

On 2026-06-02 23:21:49 -0400, Andres Freund wrote:
> On 2026-06-02 12:15:40 -0400, Melanie Plageman wrote:
> > On Mon, Apr 6, 2026 at 4:21 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Mon, Mar 30, 2026 at 11:25:55AM +0300, Yura Sokolov wrote:
> > > > Have you any recommendations for patch?
> > > >
> > > > Should I create CF item for patch?
> > >
> > > The business with unset_flag_bits is new as of HEAD, so this could
> > > qualify as an open item, to be tracked here:
> > > https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
> > 
> > There hasn't been recent activity on this thread and it is one of the
> > oldest open items. Even if we don't have time to review the test right
> > now, it is probably worth pushing the fix. Though we have just missed
> > beta1...
> 
> Ugh. I had lost track of this.  I'll get back to it as soon as I have squared
> the CI stuff away.

I'd hoped to push a patch by now, but unfortunately didn't yet, and I'm about
to take 4 days off outside of reception. So pushing it now seems unwise.

FWIW, I am inclined to just fix UnlockBufHdrExt() to first subtract and then
add the bits, rather than the other way round and document that fact. I think
that's cleaner than having to force callers to do their own bit math to
prevent bits from overlapping.

I played around a bunch with the tests, but I suspect it's better to wait with
testing this path until the write path is converted to AIO, otherwise we have
to introduce infrastructure that we'll just rip out again...

Greetings,

Andres Freund

  11. Re: BM_IO_ERROR flag is lost in TerminateBufferIO due to order of operations in UnlockBufHdrExt

    Michael Paquier <michael@paquier.xyz> — 2026-06-15T23:35:30Z

    On Mon, Jun 15, 2026 at 12:18:43PM -0400, Andres Freund wrote:
> I'd hoped to push a patch by now, but unfortunately didn't yet, and I'm about
> to take 4 days off outside of reception. So pushing it now seems unwise.

No worries and no need to rush.  You still have a lot of time ahead
for this issue, and the buildfarm is super calm currently.
--
Michael