Hi Vignesh, Thanks for catching this, and for the test. It fails on v1 as you describe.
v2 drops the flag. _hash_alloc_buckets() now extends the file before writing the WAL record, so a checkpoint either replays the record or syncs the new block. ^^^^ is aligned with current heap extend behavior. Another way is to use PG_TRY and FINALLY to reset the flag. I prefer to use same mechanism because it is easy to understand in the future. 0001 and 0002 are unchanged. Thanks, Shihao.
From 757a5ab9f191dfc7440c5fa051f24b4f3cfc6c27 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Mon, 2 Nov 2026 22:13:30 -0500 Subject: [PATCH v2 4/4] Add test for a checkpoint that starts during hash bucket allocation Add injection points at the start of a hash index split and between the file extension and the WAL record in _hash_alloc_buckets(). A new TAP test takes a base backup while a split waits at the second one, and checks that the restored index still accepts inserts. It also makes the step after the extension fail inside a savepoint. Then it checks that checkpoints do not wait, and that a retried split works. Backpatch-through: 18 --- src/backend/access/hash/hashpage.c | 4 + src/test/recovery/meson.build | 1 + .../t/062_hash_alloc_buckets_checkpoint.pl | 205 ++++++++++++++++++ 3 files changed, 210 insertions(+) create mode 100644 src/test/recovery/t/062_hash_alloc_buckets_checkpoint.pl diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index e24fbe6f4ff..3a636a13aeb 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -635,6 +635,8 @@ _hash_expandtable(Relation rel, Buffer metabuf) restart_expand: + INJECTION_POINT("hash-expandtable-start", NULL); + /* * Write-lock the meta page. It used to be necessary to acquire a * heavyweight lock to begin a split, but that is no longer required. @@ -1041,6 +1043,8 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks) smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data, false); + INJECTION_POINT("hash-alloc-buckets-before-wal", NULL); + if (RelationNeedsWAL(rel)) log_newpage(&rel->rd_locator, MAIN_FORKNUM, diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index eb3a1bc8a39..8fc36f61d64 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -70,6 +70,7 @@ tests += { 't/059_remote_apply_status_interval.pl', 't/060_multixact_crashed_updater.pl', 't/061_clog_truncate_checkpoint.pl', + 't/062_hash_alloc_buckets_checkpoint.pl', ], }, } diff --git a/src/test/recovery/t/062_hash_alloc_buckets_checkpoint.pl b/src/test/recovery/t/062_hash_alloc_buckets_checkpoint.pl new file mode 100644 index 00000000000..02cc7cb65cf --- /dev/null +++ b/src/test/recovery/t/062_hash_alloc_buckets_checkpoint.pl @@ -0,0 +1,205 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# When a hash index allocates a new splitpoint, it extends the file to the +# last block and WAL-logs that block. If a base backup's start checkpoint +# came between the two steps, and the extension came after the WAL record, +# the backup could copy the file without that block, and recovery would not +# add it back. Inserts would then fail once they need an overflow page past +# the end of the file. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use Time::HiRes qw(usleep); + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init(allows_streaming => 1); +$node->append_conf('postgresql.conf', 'autovacuum = off'); +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); + +$node->safe_psql( + 'postgres', q( + CREATE TABLE hash_tab (k int); + CREATE INDEX hash_idx ON hash_tab USING hash (k); + INSERT INTO hash_tab SELECT (random() * 1e9)::int + FROM generate_series(1, 20000); +)); + +my $splitter = $node->background_psql('postgres'); +my $splitter_pid = $splitter->query_safe('SELECT pg_backend_pid()'); +$splitter->query_safe( + q(SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-expandtable-start', 'wait'); + SELECT injection_points_attach('hash-alloc-buckets-before-wal', 'wait'); +)); + +# Insert until the index file grows, which is either a new splitpoint or a +# new overflow page. +my $insert_loop = q{ + \echo starting_inserts + DO $$ + DECLARE sz bigint := pg_relation_size('hash_idx'); + BEGIN + LOOP + INSERT INTO hash_tab VALUES ((random() * 1e9)::int); + EXIT WHEN pg_relation_size('hash_idx') <> sz; + END LOOP; + END $$; +}; +$splitter->query_until(qr/starting_inserts/, $insert_loop); + +# Each split stops at hash-expandtable-start. Run a checkpoint there, so +# that the metapage is clean when the split stops in _hash_alloc_buckets(). +# Otherwise the backup's checkpoint would wait for the metapage content lock +# until the split finishes. +my $max_attempts = $PostgreSQL::Test::Utils::timeout_default * 100; +while (1) +{ + die "timed out waiting for a splitpoint allocation" + if --$max_attempts < 0; + + my $state = $node->safe_psql('postgres', + "SELECT state || ':' || coalesce(wait_event, '') FROM pg_stat_activity WHERE pid = $splitter_pid" + ); + if ($state eq 'active:hash-expandtable-start') + { + $node->safe_psql('postgres', 'CHECKPOINT'); + $node->safe_psql('postgres', + q(SELECT injection_points_wakeup('hash-expandtable-start'))); + + # The waiter checks for the wakeup only every 100ms, and the next + # insert may come right back here. Give it time to move on. + usleep(250_000); + } + elsif ($state eq 'active:hash-alloc-buckets-before-wal') + { + last; + } + elsif ($state =~ /^idle/) + { + # The file grew because of an overflow page, try again. + $splitter->query_until(qr/starting_inserts/, $insert_loop); + } + else + { + usleep(10_000); + } +} + +# Take a base backup while the split waits between the extension and the WAL +# record, and wait until the backup either finishes, or its checkpoint waits +# for the split. +my $backup_path = $node->backup_dir . '/hash_backup'; +my ($bb_stdout, $bb_stderr) = ('', ''); +my $bb = IPC::Run::start( + [ + 'pg_basebackup', '--no-sync', + '--pgdata' => $backup_path, + '--host' => $node->host, + '--port' => $node->port, + '--checkpoint' => 'fast', + ], + '>' => \$bb_stdout, + '2>' => \$bb_stderr); + +$max_attempts = $PostgreSQL::Test::Utils::timeout_default * 10; +while ($bb->pumpable) +{ + die "timed out waiting for the base backup" if --$max_attempts < 0; + $bb->pump_nb; + last + if $node->safe_psql('postgres', + "SELECT count(*) FROM pg_stat_activity WHERE backend_type = 'checkpointer' AND wait_event = 'CheckpointDelayStart'" + ) eq '1'; + usleep(100_000); +} + +$node->safe_psql('postgres', + q(SELECT injection_points_wakeup('hash-alloc-buckets-before-wal'))); +$bb->finish or die "pg_basebackup failed: $bb_stderr"; +$splitter->query_safe( + q(SELECT injection_points_detach('hash-expandtable-start'); + SELECT injection_points_detach('hash-alloc-buckets-before-wal');)); +$splitter->quit; + +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($node, 'hash_backup', has_streaming => 1); +$standby->start; +$node->wait_for_replay_catchup($standby); +$standby->promote; + +my ($ret, $stdout, $stderr) = $standby->psql('postgres', + q(INSERT INTO hash_tab SELECT (random() * 1e9)::int + FROM generate_series(1, 200000))); +is($ret, 0, 'hash index restored from backup accepts inserts'); +is($standby->safe_psql('postgres', q(SELECT pg_relation_size('hash_idx') > 0)), + 't', 'hash index is not empty'); +$standby->stop; + +# If the step after the extension fails, and a savepoint catches the error, +# checkpoints must not wait for the rest of the transaction, and the next +# split must be able to allocate the same blocks again. +my $failer = $node->background_psql('postgres', on_error_stop => 0); +$failer->query_safe( + q(SELECT injection_points_set_local(); + SELECT injection_points_attach('hash-alloc-buckets-before-wal', 'error'); + BEGIN; + SAVEPOINT s;)); +$failer->query( + q{DO $$ + BEGIN + FOR i IN 1..3000 LOOP + INSERT INTO hash_tab SELECT (random() * 1e9)::int + FROM generate_series(1, 1000); + END LOOP; + END $$;}); +like( + $failer->{stderr}, + qr/error triggered for injection point hash-alloc-buckets-before-wal/, + 'bucket allocation failed after extending the file'); +$failer->{stderr} = ''; +$failer->query_safe('ROLLBACK TO s'); + +my $timed_out = 0; +$node->psql( + 'postgres', 'CHECKPOINT', + timeout => $PostgreSQL::Test::Utils::timeout_default, + timed_out => \$timed_out); +ok(!$timed_out, 'checkpoint does not wait after rollback to savepoint'); + +$failer->query_safe( + q(SELECT injection_points_detach('hash-alloc-buckets-before-wal'); + SELECT injection_points_attach('hash-alloc-buckets-before-wal', 'notice');)); +$failer->{stderr} = ''; +$failer->query( + q(INSERT INTO hash_tab SELECT (random() * 1e9)::int + FROM generate_series(1, 100000))); +like( + $failer->{stderr}, + qr/notice triggered for injection point hash-alloc-buckets-before-wal/, + 'bucket allocation was retried'); +unlike($failer->{stderr}, qr/ERROR/, 'retried bucket allocation succeeded'); + +$failer->query( + q(SELECT injection_points_detach('hash-alloc-buckets-before-wal'))); +$failer->{stderr} = ''; +$failer->query_safe('COMMIT'); +$failer->quit; +$node->stop; + +done_testing(); -- 2.37.1 (Apple Git-137.1)
From ec0bc9c1fa8e29660d15dee4311a9436e372c0f3 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Thu, 29 Oct 2026 22:10:14 -0400 Subject: [PATCH v2 2/4] Add test for a checkpoint that starts during CLOG truncation Add an injection point after TruncateCLOG() removes the old segments, and a TAP test that runs a checkpoint there. After crash recovery, the status of an XID in a removed segment must be NULL, not an error. Backpatch-through: 18 --- src/backend/access/transam/clog.c | 3 + src/test/recovery/Makefile | 3 +- src/test/recovery/meson.build | 1 + .../t/061_clog_truncate_checkpoint.pl | 66 +++++++++++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/061_clog_truncate_checkpoint.pl diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c index 6f7f6b86eb6..52dafb6046c 100644 --- a/src/backend/access/transam/clog.c +++ b/src/backend/access/transam/clog.c @@ -46,6 +46,7 @@ #include "storage/subsystems.h" #include "storage/sync.h" #include "utils/guc_hooks.h" +#include "utils/injection_point.h" #include "utils/wait_event.h" /* @@ -1015,6 +1016,8 @@ TruncateCLOG(TransactionId oldestXact, Oid oldestxid_datoid) /* Now we can remove the old CLOG segment(s) */ SimpleLruTruncate(XactCtl, cutoffPage); + + INJECTION_POINT("clog-truncate-after-unlink", NULL); } diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 9c4102b6b2c..edc50a41923 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -13,7 +13,8 @@ EXTRA_INSTALL=contrib/pg_prewarm \ contrib/pg_stat_statements \ contrib/test_decoding \ src/test/modules/injection_points \ - src/test/modules/test_wait_lsn + src/test/modules/test_wait_lsn \ + src/test/modules/xid_wraparound subdir = src/test/recovery top_builddir = ../../.. diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index b6ac9dd07a7..eb3a1bc8a39 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -69,6 +69,7 @@ tests += { 't/058_shutdown_crash_restart.pl', 't/059_remote_apply_status_interval.pl', 't/060_multixact_crashed_updater.pl', + 't/061_clog_truncate_checkpoint.pl', ], }, } diff --git a/src/test/recovery/t/061_clog_truncate_checkpoint.pl b/src/test/recovery/t/061_clog_truncate_checkpoint.pl new file mode 100644 index 00000000000..dc9bd63850e --- /dev/null +++ b/src/test/recovery/t/061_clog_truncate_checkpoint.pl @@ -0,0 +1,66 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# CLOG truncation removes old pg_xact segments. If a checkpoint whose redo +# pointer is after the truncation record still recorded the old oldestXid, +# recovery from it would set oldestClogXid to that value, and status lookups +# of XIDs in the removed segments would fail instead of returning NULL. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', 'autovacuum = off'); +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); +$node->safe_psql('postgres', 'CREATE EXTENSION xid_wraparound'); +my $old_xid = $node->safe_psql('postgres', 'SELECT pg_current_xact_id()'); + +# Use up more than one pg_xact segment, and freeze all databases except one. +$node->safe_psql('postgres', 'SELECT consume_xids(1100000)'); +$node->safe_psql('postgres', + 'ALTER DATABASE template0 WITH ALLOW_CONNECTIONS true'); +$node->safe_psql($_, 'VACUUM (FREEZE)') for qw(template0 template1); + +# The last vacuum truncates CLOG, and stops there. +my $vacuum = $node->background_psql('postgres', on_error_stop => 0); +$vacuum->query_safe( + q(SELECT injection_points_set_local(); + SELECT injection_points_attach('clog-truncate-after-unlink', 'wait');)); +$vacuum->query_until( + qr/starting_vacuum/, q( + \echo starting_vacuum + VACUUM (FREEZE); +)); +$node->wait_for_event('client backend', 'clog-truncate-after-unlink'); +ok(!-e $node->data_dir . '/pg_xact/0000', 'oldest pg_xact segment removed'); + +# This checkpoint's redo pointer is after the truncation record. +$node->safe_psql('postgres', 'CHECKPOINT'); + +$node->stop('immediate'); +$vacuum->{run}->finish; +$node->start; + +my ($ret, $stdout, $stderr) = + $node->psql('postgres', qq(SELECT pg_xact_status('$old_xid'::xid8))); +is($ret, 0, 'status of truncated XID can be looked up after crash recovery'); +is($stdout, '', 'status of truncated XID is NULL after crash recovery'); + +$node->stop; +done_testing(); -- 2.37.1 (Apple Git-137.1)
From 93b584aafef0aabb7a8e63243dfe3759c4cfe695 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Thu, 29 Oct 2026 21:51:30 -0400 Subject: [PATCH v2 1/4] Advance oldestXid before truncating CLOG vac_truncate_clog() wrote the CLOG truncation record and removed the old pg_xact segments, and only then called SetTransactionIdLimit(). A checkpoint whose redo pointer lands between the two steps still records the old oldestXid. Crash recovery from that checkpoint does not replay the truncation record, and StartupXLOG() sets oldestClogXid from the checkpoint's oldestXid. oldestClogXid then points into removed segments, and pg_xact_status() on an XID in that range fails with "could not access status of transaction" instead of returning NULL. The next VACUUM does not fix it, since there is no segment left to remove. The same happens when starting from a base backup or a restartpoint taken in that window. Fix by calling SetTransactionIdLimit() before TruncateCLOG(), as we already do for commit timestamps with AdvanceOldestCommitTsXid(). A checkpoint whose redo pointer is after the truncation record then sees the new oldestXid. If we crash after advancing oldestXid but before truncating, the old segments are left behind until the next truncation, which is what already happens if we crash before the truncation record. Backpatch-through: 14 --- src/backend/access/transam/xlog.c | 4 +++- src/backend/commands/vacuum.c | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 8636ad9ab2c..29237579e9e 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7941,7 +7941,9 @@ CreateCheckPoint(int flags) * We don't need to save oldestClogXid in the checkpoint, it only matters * for the short period in which clog is being truncated, and if we crash * during that we'll redo the clog truncation and fix up oldestClogXid - * there. + * there. If our redo pointer is after the clog truncation record, we see + * the new oldestXid, because vac_truncate_clog() advances it before + * truncating. */ LWLockAcquire(XidGenLock, LW_SHARED); checkPoint.nextXid = TransamVariables->nextXid; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 3a5c1efd6ac..307f409b9c4 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1992,6 +1992,16 @@ vac_truncate_clog(TransactionId frozenXID, */ AdvanceOldestCommitTsXid(frozenXID); + /* + * Update the wrap limit for GetNewTransactionId. Do this before + * truncating CLOG, so that a checkpoint starting after the truncation + * record sees the new oldestXid. Crash recovery from such a checkpoint + * does not replay the truncation, and sets oldestClogXid from the + * checkpoint's oldestXid. Note: this function will also signal the + * postmaster for an autovac cycle if needed. + */ + SetTransactionIdLimit(frozenXID, oldestxid_datoid); + /* * Truncate CLOG, multixact and CommitTs to the oldest computed value. */ @@ -2000,12 +2010,10 @@ vac_truncate_clog(TransactionId frozenXID, TruncateMultiXact(minMulti, minmulti_datoid); /* - * Update the wrap limit for GetNewTransactionId and creation of new - * MultiXactIds. Note: these functions will also signal the postmaster - * for an(other) autovac cycle if needed. XXX should we avoid possibly - * signaling twice? + * Update the wrap limit for creation of new MultiXactIds. Note: this + * function will also signal the postmaster for an(other) autovac cycle if + * needed. XXX should we avoid possibly signaling twice? */ - SetTransactionIdLimit(frozenXID, oldestxid_datoid); SetMultiXactIdLimit(minMulti, minmulti_datoid); LWLockRelease(WrapLimitsVacuumLock); -- 2.37.1 (Apple Git-137.1)
From 7a87b3ff88f807f6f3162b1141db176b89b963e6 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Mon, 2 Nov 2026 22:12:02 -0500 Subject: [PATCH v2 3/4] Extend hash index before WAL-logging new bucket pages _hash_alloc_buckets() WAL-logs the last block of a new splitpoint with log_newpage(), and then extends the file with smgrextend(). Nothing kept a checkpoint from setting its redo pointer between the two steps. Recovery from such a checkpoint does not replay the log_newpage() record, and redo of the later records only extends the file up to the bucket pages that are actually used. If the extension did not make it into the checkpoint, the file ends up shorter than the metapage says. This can happen if the fsync request of smgrextend() arrives after the checkpoint has absorbed its requests, followed by an OS crash. It can also happen with a base backup whose start checkpoint lands in the window, if the index file is copied before the extension. Inserts then fail with "access to noncontiguous page in hash index" once an overflow page is needed past the end of the file, until the index is rebuilt. Fix by doing the two steps in the other order. If a checkpoint sets its redo pointer before the WAL record, recovery replays the record. If it sets it after, the extension and its fsync request came first. The checkpoint then syncs the new block, and a base backup sees it. Nothing uses the new page yet. So it is fine to write it before its WAL record, as heap extension does. A crash in between leaves the file longer than the metapage says, which the hash README allows. Backpatch-through: 14 --- src/backend/access/hash/hashpage.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index a0ab2528167..e24fbe6f4ff 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -1025,6 +1025,22 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks) ovflopaque->hasho_flag = LH_UNUSED_PAGE; ovflopaque->hasho_page_id = HASHO_PAGE_ID; + /* + * Extend the file before writing the WAL record. If a checkpoint sets + * its redo pointer before the record, recovery replays the record and + * extends the file again. If it sets it after the record, the extension + * is already done, and smgrextend() has already registered the fsync + * request. The checkpoint then syncs the new block, and a base backup + * started by it sees the block. So we must not skip the fsync here. + * + * Nothing uses the new page yet, so it is fine to write it before its WAL + * record. If we crash in between, the file is longer than the metapage + * says, which is allowed; see the README. + */ + PageSetChecksum(page, lastblock); + smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data, + false); + if (RelationNeedsWAL(rel)) log_newpage(&rel->rd_locator, MAIN_FORKNUM, @@ -1032,10 +1048,6 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks) zerobuf.data, true); - PageSetChecksum(page, lastblock); - smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data, - false); - return true; } -- 2.37.1 (Apple Git-137.1)
