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

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

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

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

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

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

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

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

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

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

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

Hi,

On 2026-06-15 12:18:43 -0400, Andres Freund wrote:

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

Pushed that way, and marked the open item as resolved. I want to introduce
some more tests around this, but I think that's 20 material.

Thanks again for the report!

Greetings,

Andres