From 33501fe1747a460c2e1ec57edd3ebaeacc07dec7 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Wed, 11 Nov 2026 20:58:41 +0000 Subject: [PATCH v6 1/4] Fix hash index redo reading a torn primary bucket page Replay of XLOG_HASH_DELETE, XLOG_HASH_MOVE_PAGE_CONTENTS and XLOG_HASH_SQUEEZE_PAGE reads the primary bucket page only to take a cleanup lock on it, so that no scan on a hot standby runs in the bucket while tuples are removed or moved. VACUUM holds only a pin on that page, so the record does not modify it and carries no full-page image of it; redo reads it from disk with RBM_NORMAL. If the page is torn on disk and the record that repairs it with a full-page image comes later in the WAL, recovery fails when it reaches one of these records before the image, because the RBM_NORMAL read of the torn page raises "invalid page". This needs data checksums (or full_page_writes catching the tear) to be detected, and the right interleaving of a VACUUM that removes tuples from an overflow page while the primary bucket page is unmodified, followed by a later modification of the primary page. Robert Haas noted this torn-page hazard during the original hash WAL work in 2017; redo was switched to XLogReadBufferForRedoExtended() so it restores an image when the record has one, but these records register the primary bucket page pin-only with REGBUF_NO_IMAGE, so there is no image to restore and the RBM_NORMAL read still reaches the torn page. The cleanup lock matters only to serialize against hot-standby scans. When hot standby is not active no such scan can exist, so the read and lock are unnecessary; skip them. That also avoids reading the page in the only window where its torn on-disk copy has not yet been repaired, since the repairing image always precedes the point where hot standby becomes active. nbtree and gist already gate redo cleanup-lock work on InHotStandby for the same reason. Reported-by: shihao zhong Author: Greg Burd Co-authored-by: Vignesh C Suggested-by: Amit Kapila Discussion: https://postgr.es/m/CAGRkXqT_AiwcsWZg5hnR1L2oWO8UpQpkL0iAjoZR+bey=GOWaQ@mail.gmail.com Discussion: https://postgr.es/m/CAA4eK1LwZk0MaK0LncZz6hsqcHnamfJB47nA8xuwRp4CyLFKzw@mail.gmail.com --- src/backend/access/hash/hash_xlog.c | 42 ++++-- src/test/recovery/meson.build | 1 + ...68a_hash_torn_primary_page_delete_first.pl | 142 ++++++++++++++++++ 3 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 src/test/recovery/t/068a_hash_torn_primary_page_delete_first.pl diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c index e9a2b9aa9a7..35791b087be 100644 --- a/src/backend/access/hash/hash_xlog.c +++ b/src/backend/access/hash/hash_xlog.c @@ -494,10 +494,18 @@ hash_xlog_move_page_contents(XLogReaderState *record) else { /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. + * The cleanup lock on the primary bucket page serializes replay + * against scans on a hot standby, which pin the page for their whole + * duration. When hot standby is not active no such scan can exist, + * so we need not read or lock the page. Skipping the read also + * sidesteps a torn copy of the page whose repairing full-page image + * comes later in the WAL: that hazard exists only before consistency + * is reached, which is before hot standby becomes active, so once + * InHotStandby is true any earlier image has already been replayed. + * We do not use the page contents here in any case. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (InHotStandby) + (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); action = XLogReadBufferForRedo(record, 1, &writebuf); } @@ -621,10 +629,18 @@ hash_xlog_squeeze_page(XLogReaderState *record) else { /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. + * The cleanup lock on the primary bucket page serializes replay + * against scans on a hot standby, which pin the page for their whole + * duration. When hot standby is not active no such scan can exist, + * so we need not read or lock the page. Skipping the read also + * sidesteps a torn copy of the page whose repairing full-page image + * comes later in the WAL: that hazard exists only before consistency + * is reached, which is before hot standby becomes active, so once + * InHotStandby is true any earlier image has already been replayed. + * We do not use the page contents here in any case. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (InHotStandby) + (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); if (xldata->ntups > 0 || xldata->is_prev_bucket_same_wrt) action = XLogReadBufferForRedo(record, 1, &writebuf); @@ -852,10 +868,18 @@ hash_xlog_delete(XLogReaderState *record) else { /* - * we don't care for return value as the purpose of reading bucketbuf - * is to ensure a cleanup lock on primary bucket page. + * The cleanup lock on the primary bucket page serializes replay + * against scans on a hot standby, which pin the page for their whole + * duration. When hot standby is not active no such scan can exist, + * so we need not read or lock the page. Skipping the read also + * sidesteps a torn copy of the page whose repairing full-page image + * comes later in the WAL: that hazard exists only before consistency + * is reached, which is before hot standby becomes active, so once + * InHotStandby is true any earlier image has already been replayed. + * We do not use the page contents here in any case. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (InHotStandby) + (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); action = XLogReadBufferForRedo(record, 1, &deletebuf); } diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index b6ac9dd07a7..06b649be2aa 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/068a_hash_torn_primary_page_delete_first.pl', ], }, } diff --git a/src/test/recovery/t/068a_hash_torn_primary_page_delete_first.pl b/src/test/recovery/t/068a_hash_torn_primary_page_delete_first.pl new file mode 100644 index 00000000000..e61a0b98885 --- /dev/null +++ b/src/test/recovery/t/068a_hash_torn_primary_page_delete_first.pl @@ -0,0 +1,142 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test crash recovery when the primary bucket page of a hash bucket is torn, +# and the full-page image that repairs it comes after the record that reads it. +# + +use strict; +use warnings FATAL => 'all'; + +use IPC::Run; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init; +$node->append_conf( + 'postgresql.conf', qq{ +autovacuum = off +fsync = off +full_page_writes = on +checkpoint_timeout = 1h +}); +$node->start; + +is($node->safe_psql('postgres', 'SHOW data_checksums'), + 'on', 'premise: data checksums are on'); + +# One bucket receives every key: a primary page holding about 407 entries and +# an overflow page with the rest. +$node->safe_psql( + 'postgres', q{ +CREATE TABLE t (id int, k int) WITH (autovacuum_enabled = false); +CREATE INDEX t_i ON t USING hash (k); +INSERT INTO t SELECT i, 1 FROM generate_series(1, 600) i; +}); +$node->safe_psql('postgres', 'CHECKPOINT'); +my $redo = $node->safe_psql('postgres', + 'SELECT redo_lsn FROM pg_control_checkpoint()'); + +# The entries of ids 451-600 are on the overflow page, those of ids 1-50 on the +# primary page. The DELETE for the overflow page comes first; the VACUUM of +# the primary page then changes it for the first time since the checkpoint, so +# its record carries the full-page image of the page. +$node->safe_psql('postgres', 'DELETE FROM t WHERE id > 450'); +$node->safe_psql('postgres', 'VACUUM t'); +$node->safe_psql('postgres', 'DELETE FROM t WHERE id <= 50'); +$node->safe_psql('postgres', 'VACUUM t'); + +my $relpath = $node->safe_psql('postgres', + q{SELECT pg_relation_filepath('t_i')}); +my $bs = $node->safe_psql('postgres', 'SHOW block_size'); +$node->safe_psql('postgres', 'SELECT pg_switch_wal()'); + +# Read the Hash records after the checkpoint. +my ($stdout, $stderr); +IPC::Run::run( + [ + 'pg_waldump', + '--path' => $node->data_dir . '/pg_wal', + '--start' => $redo, + '--rmgr' => 'Hash', + ], + '>' => \$stdout, + '2>' => \$stderr); +my @records = grep { /^rmgr: Hash/ } split(/^(?=rmgr: )/m, $stdout); + +# The primary bucket page's block number is not a registered block of the +# overflow-page DELETE (the record carries it only as appended main data), so +# find it from a record that does register it: a DELETE or SQUEEZE_PAGE marked +# is_primary T references the primary bucket page as its first block. +my $pblk; +for my $r (@records) +{ + if ($r =~ /desc: (?:DELETE|SQUEEZE_PAGE) .*is_primary T/ + && $r =~ /blkref #0: rel \S+ blk (\d+)/) + { + $pblk = $1; + last; + } +} +ok(defined $pblk, 'premise: found the primary bucket page') + or diag $stdout; + +# The overflow-page DELETE (is_primary F) reads the primary bucket page +# pin-only to take the cleanup lock; it must come before the primary page's +# first full-page image. +my ($idx_delete) = + grep { $records[$_] =~ /desc: DELETE .*is_primary F/ } 0 .. $#records; +ok(defined $idx_delete, + 'premise: VACUUM wrote a DELETE for an overflow page') + or diag $stdout; + +# Where the first full-page image of the primary page is. +my ($idx_fpi) = + defined $pblk + ? grep { $records[$_] =~ /blkref #\d+: rel \S+ blk $pblk FPW/ } + 0 .. $#records + : (); +ok(defined $idx_fpi, + 'premise: the primary page got a full-page image after the checkpoint') + or diag $stdout; +if (defined $idx_delete && defined $idx_fpi) +{ + cmp_ok($idx_delete, '<', $idx_fpi, + 'premise: the DELETE comes before the image of the primary page'); +} + +$node->stop('immediate'); + +# Simulate a torn write of the primary page: keep the first half of its block +# and overwrite the second half. +if (defined $pblk) +{ + my $path = $node->data_dir . '/' . $relpath; + open(my $fh, '+<:raw', $path) or die "could not open $path: $!"; + sysseek($fh, $pblk * $bs + $bs / 2, 0) or die "seek: $!"; + my $junk = join('', map { chr(int(rand(256))) } 1 .. $bs / 2); + syswrite($fh, $junk) == $bs / 2 or die "write: $!"; + close($fh); +} + +my $started = $node->start(fail_ok => 1); +ok($started, 'crash recovery succeeds'); +if ($started) +{ + is( $node->safe_psql( + 'postgres', + q{SET enable_seqscan = off; SET enable_bitmapscan = off; + SELECT count(*) FROM t WHERE k = 1}), + '400', + 'the hash index gives the right result after recovery'); + $node->stop; +} +else +{ + my $log = slurp_file($node->logfile); + diag("the server did not start; the log has:\n" + . join('', grep { /invalid page|FATAL|WAL redo/ } split(/^/m, $log))); +} + +done_testing(); -- 2.47.3