From: Palak Chaturvedi Subject: [PATCH] buffermgr: fix tagged-buffer eviction during shrink --- src/backend/storage/buffer/bufmgr.c | 28 +++-- src/test/buffermgr/meson.build | 1 src/test/buffermgr/t/003_resize_failures.pl | 2 src/test/buffermgr/t/006_evict_mid_io_race.pl | 153 +++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 9 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2163,6 +2163,13 @@ AsyncReadBuffers(ReadBuffersOperation *operation, int *nblocks_progress) pgaio_io_set_flag(ioh, ioh_flags); + /* + * Test hook: stall after BM_TAG_VALID is set but before the read + * issues. With IOMETHOD_SYNC this is the only window where the + * buffer is visible (BM_TAG_VALID) but not yet valid (BM_VALID). + */ + INJECTION_POINT("start-read-buffers-before-readv", NULL); + /* --- * Even though we're trying to issue IO asynchronously, track the time * in smgrstartreadv(): @@ -9152,26 +9159,31 @@ EvictExtraBuffers(int targetNBuffers, int currentNBuffers) buf_state = pg_atomic_read_u64(&desc->state); /* - * Nobody is expected to allocate new buffers while resizing is going - * on hence unlocked precheck should be safe and saves some cycles. + * A tagged buffer must be evicted even if its data is not yet + * valid. Skipping it could leave a mapping to a removed buffer. */ - if (!(buf_state & BM_VALID)) + if (!(buf_state & BM_TAG_VALID)) continue; ResourceOwnerEnlarge(CurrentResourceOwner); ReservePrivateRefCountEntry(); - LockBufHdr(desc); + buf_state = LockBufHdr(desc); /* - * Now that we have locked buffer descriptor, make sure that the - * buffer without valid data has been skipped above. + * Concurrent invalidation may have cleared the tag since the + * unlocked precheck. */ - Assert(buf_state & BM_VALID); + if (!(buf_state & BM_TAG_VALID)) + { + UnlockBufHdr(desc); + continue; + } if (!EvictUnpinnedBufferInternal(desc, &buffer_flushed)) { - elog(WARNING, "could not remove buffer %u, it is pinned", buf); + ereport(WARNING, + (errmsg("could not evict buffer %u", buf))); result = false; break; } diff --git a/src/test/buffermgr/meson.build b/src/test/buffermgr/meson.build --- a/src/test/buffermgr/meson.build +++ b/src/test/buffermgr/meson.build @@ -25,6 +25,7 @@ tests += { 't/003_resize_failures.pl', 't/004_resize_with_syslogger.pl', 't/005_resize_unsupported.pl', + 't/006_evict_mid_io_race.pl', 't/010_stress_resize_buffer.pl', 't/011_stress_drop_relation_buffers.pl', 't/012_stress_drop_database_buffers.pl', diff --git a/src/test/buffermgr/t/003_resize_failures.pl b/src/test/buffermgr/t/003_resize_failures.pl --- a/src/test/buffermgr/t/003_resize_failures.pl +++ b/src/test/buffermgr/t/003_resize_failures.pl @@ -83,7 +83,7 @@ my $log_offset = -s $node->logfile; is($node->safe_psql('postgres', "SELECT pg_resize_shared_buffers()"), 'f', "shrink returns false when a buffer to be evicted is pinned"); -ok($node->log_contains(qr/could not remove buffer $pinned_buf, it is pinned/, $log_offset), +ok($node->log_contains(qr/could not evict buffer $pinned_buf\b/, $log_offset), "log reports the pinned buffer that blocked eviction"); ok($node->log_contains(qr/failed to evict extra buffers during shrinking/, $log_offset), "log reports the eviction failure"); diff --git a/src/test/buffermgr/t/006_evict_mid_io_race.pl b/src/test/buffermgr/t/006_evict_mid_io_race.pl new file mode 100644 --- /dev/null +++ b/src/test/buffermgr/t/006_evict_mid_io_race.pl @@ -0,0 +1,153 @@ +# Copyright (c) 2025-2026, PostgreSQL Global Development Group +# +# Check that shrink rolls back while a tagged buffer above the target has +# an unfinished read, then succeeds after the reader completes. + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if (!$ENV{enable_injection_points} || $ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => "test requires injection points"; +} + +my $initial_nbuffers = 64; +my $grown_nbuffers = 128; +my $max_nbuffers = 256; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', + 'shared_preload_libraries = injection_points'); +$node->append_conf('postgresql.conf', "shared_buffers = $initial_nbuffers"); +$node->append_conf('postgresql.conf', "max_shared_buffers = $max_nbuffers"); +$node->append_conf('postgresql.conf', 'io_method = sync'); +$node->append_conf('postgresql.conf', 'huge_pages = off'); +$node->start; + +if ($node->safe_psql('postgres', 'SHOW have_resizable_shmem') ne 'on') +{ + plan skip_all => "resizable shared memory not supported by this build"; +} + +$node->safe_psql('postgres', "CREATE EXTENSION pg_buffercache"); +$node->safe_psql('postgres', "CREATE EXTENSION injection_points"); + +$node->safe_psql('postgres', + "CREATE TABLE evict_race AS SELECT generate_series(1,2) AS i"); +$node->safe_psql('postgres', "CHECKPOINT"); + +# Grow the pool so we have buffers above the initial size. +$node->safe_psql('postgres', + "ALTER SYSTEM SET shared_buffers = '$grown_nbuffers'"); +$node->safe_psql('postgres', "SELECT pg_reload_conf()"); +$node->safe_psql('postgres', "SELECT pg_resize_shared_buffers()"); + +my $current = $node->safe_psql('postgres', + "SELECT current_nbuffers FROM pg_get_buffer_resize_status()"); +is($current, "$grown_nbuffers", "pool grown to $grown_nbuffers"); + +my $reader = $node->background_psql('postgres'); +my $reader_pid = $reader->query_safe("SELECT pg_backend_pid()", verbose => 0); +chomp $reader_pid; + +# Warm catalogs before attaching the injection point so that the reader waits +# while reading the table, not a catalog. +$reader->query_safe("SELECT * FROM evict_race", verbose => 0); + +$reader->query_safe("SELECT injection_points_set_local()", verbose => 0); +$reader->query_safe( + "SELECT injection_points_attach('start-read-buffers-before-readv', 'wait')", + verbose => 0); + +my $injector = $node->background_psql('postgres'); + +# Evict the victim relation's buffers. +$node->safe_psql('postgres', qq{ + SELECT count(*) FROM pg_buffercache b, + LATERAL pg_buffercache_evict(b.bufferid) e + WHERE b.relfilenode = (SELECT relfilenode FROM pg_class + WHERE relname = 'evict_race') +}); + +# Evict low-numbered buffers; check the actual victim slot below. +$node->safe_psql('postgres', qq{ + SELECT count(*) FROM pg_buffercache b, + LATERAL pg_buffercache_evict(b.bufferid) e + WHERE b.bufferid <= $initial_nbuffers + AND b.relfilenode IS NOT NULL +}); + +# Wait before issuing the table read, with BM_TAG_VALID set and BM_VALID clear. +$reader->query_until(qr/READER_STARTED/, + "\\echo READER_STARTED\nSELECT * FROM evict_race;\n"); + +$node->poll_query_until('postgres', qq{ + SELECT EXISTS ( + SELECT 1 + FROM pg_stat_activity + WHERE pid = $reader_pid + AND wait_event = 'start-read-buffers-before-readv') +}) + or die "reader never reached start-read-buffers-before-readv"; + +my $victim_bufid = $node->safe_psql('postgres', q{ + SELECT b.bufferid + 1 + FROM pg_buffercache_lookup_table b JOIN pg_database d + ON b.database = d.oid AND b.tablespace = d.dattablespace + WHERE d.datname = current_database() + AND b.relfilenode = pg_relation_filenode('evict_race') + AND b.forknum = 0 AND b.blocknum = 0 +}); +like($victim_bufid, qr/^[0-9]+$/, 'one mapping for the victim page'); +BAIL_OUT('victim mapping missing or ambiguous') + unless $victim_bufid =~ /^[0-9]+$/; +cmp_ok($victim_bufid, '>', $initial_nbuffers, + 'victim is above the shrink target'); +BAIL_OUT('victim is outside the range being removed') + unless $victim_bufid > $initial_nbuffers; + +# Shrink back to original size. +$node->safe_psql('postgres', + "ALTER SYSTEM SET shared_buffers = '$initial_nbuffers'"); +$node->safe_psql('postgres', "SELECT pg_reload_conf()"); + +my $log_offset = -s $node->logfile; +my $shrink_result = $node->safe_psql('postgres', + 'SELECT pg_resize_shared_buffers()'); + +is($shrink_result, 'f', 'shrink fails during the read'); +BAIL_OUT('shrink did not roll back') unless $shrink_result eq 'f'; +ok($node->log_contains( + qr/could not evict buffer \Q$victim_bufid\E\b/, $log_offset), + 'shrink reports the victim buffer'); +is($node->safe_psql('postgres', q{ + SELECT active_nbuffers, current_nbuffers, target_nbuffers, resizer_pid + FROM pg_get_buffer_resize_status() +}), "$grown_nbuffers|$grown_nbuffers|$grown_nbuffers|0", + 'failed shrink restores the resize state'); + +$injector->query_safe( + "SELECT injection_points_detach('start-read-buffers-before-readv')", + verbose => 0); +$injector->query_safe( + "SELECT injection_points_wakeup('start-read-buffers-before-readv')", + verbose => 0); +is($reader->query_safe(''), "1\n2", 'reader completes successfully'); +$reader->quit; +$injector->quit; + +is($node->safe_psql('postgres', 'SELECT pg_resize_shared_buffers()'), + 't', 'shrink succeeds after the read completes'); +is($node->safe_psql('postgres', q{ + SELECT active_nbuffers, current_nbuffers, target_nbuffers, resizer_pid + FROM pg_get_buffer_resize_status() +}), "$initial_nbuffers|$initial_nbuffers|$initial_nbuffers|0", + 'pool shrunk to target and resizer released'); +is($node->safe_psql('postgres', 'SELECT count(*) FROM evict_race'), + '2', 'table remains readable after shrink'); + +done_testing();