From 5cbaa035027d4f6c57d561ca4f9c5161f7a49471 Mon Sep 17 00:00:00 2001 From: Yura Sokolov Date: Fri, 27 Mar 2026 20:11:06 +0300 Subject: [PATCH v2 1/3] bufmgr: Fix possibility to set BM_IO_ERROR Previously it couldn't be set because TerminateBufferIO added BM_IO_ERROR to unset_flag_bits unconditionally, and UnlockBufHdrExt applied unset_bits after set_bits. Fix by not setting BM_IO_ERROR into unset_flag_bits if it is present in set_flag_bits. Also protect from possible similar errors by adding assertion to UnlockBufHdrExt unset_bits and set_bits have no bits in common. Modify src/test/modules/test_aio/t/001_aio.pl test_io_error to check presence of BM_IO_ERROR. --- src/backend/storage/buffer/bufmgr.c | 4 +-- src/include/storage/buf_internals.h | 1 + src/test/modules/test_aio/t/001_aio.pl | 20 ++++++++++++++ src/test/modules/test_aio/test_aio--1.0.sql | 4 +++ src/test/modules/test_aio/test_aio.c | 30 +++++++++++++++++++++ 5 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index cd21ae3fc36..a81949aca7c 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -7347,8 +7347,8 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag_bits, Assert(buf_state & BM_IO_IN_PROGRESS); unset_flag_bits |= BM_IO_IN_PROGRESS; - /* Clear earlier errors, if this IO failed, it'll be marked again */ - unset_flag_bits |= BM_IO_ERROR; + /* Clear earlier errors, unless this IO failed as well */ + unset_flag_bits |= BM_IO_ERROR & ~set_flag_bits; if (clear_dirty) unset_flag_bits |= BM_DIRTY | BM_CHECKPOINT_NEEDED; diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index ad1b7b2216a..93887cea46d 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -475,6 +475,7 @@ UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state, uint64 set_bits, uint64 unset_bits, int refcount_change) { + Assert(!(set_bits & unset_bits)); for (;;) { uint64 buf_state = old_buf_state; diff --git a/src/test/modules/test_aio/t/001_aio.pl b/src/test/modules/test_aio/t/001_aio.pl index 63cadd64c15..a48b0ddfba8 100644 --- a/src/test/modules/test_aio/t/001_aio.pl +++ b/src/test/modules/test_aio/t/001_aio.pl @@ -344,6 +344,8 @@ SELECT modify_rel_block('tmp_corr', 1, corrupt_header=>true); $tblname eq 'tbl_corr' ? qr/invalid page in block 1 of relation "base\/\d+\/\d+/ : qr/invalid page in block 1 of relation "base\/\d+\/t\d+_\d+/; + # BM_IO_ERROR and BM_TAG_VALID should be set + my $debug_print_re = qr/blockNum=1, flags=0x.?a000000, refcount=0/; # verify the error is reported in custom C code psql_like( @@ -354,6 +356,12 @@ SELECT modify_rel_block('tmp_corr', 1, corrupt_header=>true); qr/^$/, $invalid_page_re); + psql_like( + $io_method, $psql, + "validate flags of $tblname page after read_rel_block_ll()", + qq(SELECT debug_print_rel_block('$tblname', 1)), + $debug_print_re, qr/^$/); + # verify the error is reported for bufmgr reads, seq scan psql_like( $io_method, $psql, @@ -361,6 +369,12 @@ SELECT modify_rel_block('tmp_corr', 1, corrupt_header=>true); qq(SELECT count(*) FROM $tblname), qr/^$/, $invalid_page_re); + psql_like( + $io_method, $psql, + "validate flags of $tblname page after read_rel_block_ll()", + qq(SELECT debug_print_rel_block('$tblname', 1)), + $debug_print_re, qr/^$/); + # verify the error is reported for bufmgr reads, tid scan psql_like( $io_method, @@ -369,6 +383,12 @@ SELECT modify_rel_block('tmp_corr', 1, corrupt_header=>true); qq(SELECT count(*) FROM $tblname WHERE ctid = '(1, 1)'), qr/^$/, $invalid_page_re); + + psql_like( + $io_method, $psql, + "validate flags of $tblname page after read_rel_block_ll()", + qq(SELECT debug_print_rel_block('$tblname', 1)), + $debug_print_re, qr/^$/); } $psql->quit(); diff --git a/src/test/modules/test_aio/test_aio--1.0.sql b/src/test/modules/test_aio/test_aio--1.0.sql index 762ac29512f..cb168e2e08f 100644 --- a/src/test/modules/test_aio/test_aio--1.0.sql +++ b/src/test/modules/test_aio/test_aio--1.0.sql @@ -37,6 +37,10 @@ CREATE FUNCTION evict_rel(rel regclass) RETURNS pg_catalog.void STRICT AS 'MODULE_PATHNAME' LANGUAGE C; +CREATE FUNCTION debug_print_rel_block(rel regclass, blockno int) +RETURNS pg_catalog.text STRICT +AS 'MODULE_PATHNAME' LANGUAGE C; + CREATE FUNCTION invalidate_rel_block(rel regclass, blockno int) RETURNS pg_catalog.void STRICT AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c index a8267192cb7..a89cbd5786d 100644 --- a/src/test/modules/test_aio/test_aio.c +++ b/src/test/modules/test_aio/test_aio.c @@ -597,6 +597,36 @@ evict_rel(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +PG_FUNCTION_INFO_V1(debug_print_rel_block); +Datum +debug_print_rel_block(PG_FUNCTION_ARGS) +{ + Oid relid = PG_GETARG_OID(0); + BlockNumber blkno = PG_GETARG_UINT32(1); + Relation rel; + PrefetchBufferResult pr; + Buffer buf; + char *desc = NULL; + + rel = relation_open(relid, AccessExclusiveLock); + + /* + * This is a gross hack, but there's no other API exposed that allows to + * get a buffer ID without actually reading the block in. + */ + pr = PrefetchBuffer(rel, MAIN_FORKNUM, blkno); + buf = pr.recent_buffer; + + if (BufferIsValid(buf)) + desc = DebugPrintBufferRefcount(buf); + + relation_close(rel, AccessExclusiveLock); + + if (desc == NULL) + PG_RETURN_NULL(); + PG_RETURN_TEXT_P(cstring_to_text(desc)); +} + PG_FUNCTION_INFO_V1(buffer_create_toy); Datum buffer_create_toy(PG_FUNCTION_ARGS) -- 2.51.0