From 8423b47a045ee3626be5f4ffd518d55fd5a9afe9 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Thu, 12 Nov 2026 12:58:59 +0000 Subject: [PATCH v7 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 happens both in crash recovery (the image is in WAL generated after the last checkpoint) and on a standby started from a base backup whose copy of the page is torn (the image is in WAL generated during the backup). 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, which can run only once the standby accepts queries, that is once recovery has reached consistency. Until then the read and lock are unnecessary, so skip them when HotStandbyActiveInReplay() is false. That also avoids reading the page in the window where its torn on-disk copy has not yet been repaired, since the repairing image always precedes the consistency point. nbtree and gist already gate redo cleanup-lock work on the hot-standby state 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 | 43 ++++- src/backend/access/transam/xlogrecovery.c | 3 +- src/include/access/xlogrecovery.h | 1 + src/test/recovery/meson.build | 2 + ...68a_hash_torn_primary_page_delete_first.pl | 142 +++++++++++++++ .../t/068b_hash_torn_primary_page_standby.pl | 171 ++++++++++++++++++ 6 files changed, 351 insertions(+), 11 deletions(-) create mode 100644 src/test/recovery/t/068a_hash_torn_primary_page_delete_first.pl create mode 100644 src/test/recovery/t/068b_hash_torn_primary_page_standby.pl diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c index e9a2b9aa9a7..e1318bf97db 100644 --- a/src/backend/access/hash/hash_xlog.c +++ b/src/backend/access/hash/hash_xlog.c @@ -17,6 +17,7 @@ #include "access/bufmask.h" #include "access/hash.h" #include "access/hash_xlog.h" +#include "access/xlogrecovery.h" #include "access/xlogutils.h" #include "storage/standby.h" @@ -494,10 +495,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. Scans can run only once hot standby accepts queries, so + * until then 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, and queries are accepted only after that, + * so once HotStandbyActiveInReplay() is true any earlier image has + * already been replayed. We do not use the page contents here. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (HotStandbyActiveInReplay()) + (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); action = XLogReadBufferForRedo(record, 1, &writebuf); } @@ -621,10 +630,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. Scans can run only once hot standby accepts queries, so + * until then 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, and queries are accepted only after that, + * so once HotStandbyActiveInReplay() is true any earlier image has + * already been replayed. We do not use the page contents here. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (HotStandbyActiveInReplay()) + (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 +869,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. Scans can run only once hot standby accepts queries, so + * until then 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, and queries are accepted only after that, + * so once HotStandbyActiveInReplay() is true any earlier image has + * already been replayed. We do not use the page contents here. */ - (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); + if (HotStandbyActiveInReplay()) + (void) XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &bucketbuf); action = XLogReadBufferForRedo(record, 1, &deletebuf); } diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 52ac6d58423..e091a3e178b 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -390,7 +390,6 @@ static int XLogFileReadAnyTLI(XLogSegNo segno, XLogSource source); static bool CheckForStandbyTrigger(void); static void SetPromoteIsTriggered(void); -static bool HotStandbyActiveInReplay(void); static void SetCurrentChunkStartTime(TimestampTz xtime); static void SetLatestXTime(TimestampTz xtime); @@ -4589,7 +4588,7 @@ HotStandbyActive(void) * Like HotStandbyActive(), but to be used only in WAL replay code, * where we don't need to ask any other process what the state is. */ -static bool +bool HotStandbyActiveInReplay(void) { Assert(AmStartupProcess() || !IsPostmasterEnvironment); diff --git a/src/include/access/xlogrecovery.h b/src/include/access/xlogrecovery.h index 8786b6d3a8e..11d3eadef3e 100644 --- a/src/include/access/xlogrecovery.h +++ b/src/include/access/xlogrecovery.h @@ -216,6 +216,7 @@ extern void ShutdownWalRecovery(void); extern void RemovePromoteSignalFiles(void); extern bool HotStandbyActive(void); +extern bool HotStandbyActiveInReplay(void); extern XLogRecPtr GetXLogReplayRecPtr(TimeLineID *replayTLI); extern RecoveryPauseState GetRecoveryPauseState(void); extern void SetRecoveryPause(bool recoveryPause); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index b6ac9dd07a7..e7ff07ca880 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -69,6 +69,8 @@ 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', + 't/068b_hash_torn_primary_page_standby.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(); diff --git a/src/test/recovery/t/068b_hash_torn_primary_page_standby.pl b/src/test/recovery/t/068b_hash_torn_primary_page_standby.pl new file mode 100644 index 00000000000..d9a548a931e --- /dev/null +++ b/src/test/recovery/t/068b_hash_torn_primary_page_standby.pl @@ -0,0 +1,171 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test a standby that starts from a base backup whose copy of 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 File::Path qw(rmtree); +use IPC::Run; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1); +$primary->append_conf( + 'postgresql.conf', qq{ +autovacuum = off +fsync = off +full_page_writes = on +checkpoint_timeout = 1h +}); +$primary->start; + +is($primary->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. +$primary->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; +}); +my $relpath = $primary->safe_psql('postgres', + q{SELECT pg_relation_filepath('t_i')}); +my $bs = $primary->safe_psql('postgres', 'SHOW block_size'); + +# Start a low-level backup, which does a checkpoint, and copy the files. +my $backup_name = 'torn'; +my $backup_dir = $primary->backup_dir . '/' . $backup_name; +my $psql = $primary->background_psql('postgres'); +$psql->query_safe("SET client_min_messages TO WARNING"); +$psql->set_query_timer_restart; +$psql->query_safe("select pg_backup_start('torn primary page', true)"); +my $redo = $primary->safe_psql('postgres', + 'SELECT redo_lsn FROM pg_control_checkpoint()'); + +PostgreSQL::Test::RecursiveCopy::copypath($primary->data_dir, $backup_dir); +unlink("$backup_dir/postmaster.pid") + or BAIL_OUT("unable to unlink $backup_dir/postmaster.pid"); +unlink("$backup_dir/postmaster.opts") + or BAIL_OUT("unable to unlink $backup_dir/postmaster.opts"); +rmtree("$backup_dir/pg_wal") + or BAIL_OUT("unable to unlink contents of $backup_dir/pg_wal"); +mkdir("$backup_dir/pg_wal"); + +# While the backup is in progress: 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 of the backup, so its record carries the +# full-page image of the page. +$primary->safe_psql('postgres', 'DELETE FROM t WHERE id > 450'); +$primary->safe_psql('postgres', 'VACUUM t'); +$primary->safe_psql('postgres', 'DELETE FROM t WHERE id <= 50'); +$primary->safe_psql('postgres', 'VACUUM t'); +$primary->safe_psql('postgres', 'SELECT pg_switch_wal()'); + +# Read the Hash records after the redo pointer of the backup. +my ($stdout, $stderr); +IPC::Run::run( + [ + 'pg_waldump', + '--path' => $primary->data_dir . '/pg_wal', + '--start' => $redo, + '--rmgr' => 'Hash', + ], + '>' => \$stdout, + '2>' => \$stderr); +my @records = grep { /^rmgr: Hash/ } split(/^(?=rmgr: )/m, $stdout); + +# The primary page: a DELETE or SQUEEZE_PAGE marked is_primary T has it as +# block 1, the page that is changed. +my $pblk; +for my $r (@records) +{ + if ($r =~ /desc: (?:DELETE|SQUEEZE_PAGE) .*is_primary T/ + && $r =~ /blkref #[01]: rel \S+ blk (\d+)/) + { + $pblk = $1; + last; + } +} +ok(defined $pblk, 'premise: found the primary bucket page') + or diag $stdout; + +# The first DELETE for an overflow page. +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, relative to the +# DELETE for the overflow page. +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'); +} + +# The copy of the primary page in the backup is torn: keep the first half of +# its block and overwrite the second half. +if (defined $pblk) +{ + my $path = "$backup_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); +} + +# End the backup, and save backup_label into the backup. +my $backup_label = $psql->query_safe("select labelfile from pg_backup_stop()"); +$psql->quit; +open(my $fh, '>', "$backup_dir/backup_label") + or BAIL_OUT("unable to create $backup_dir/backup_label: $!"); +print $fh $backup_label; +close($fh); + +# Start a hot standby from the backup. Replay starts at the redo pointer of +# the backup, replays the DELETE for the overflow page first, and reaches the +# consistency point only after the end of the backup, after the full-page +# image of the primary page. +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($primary, $backup_name, has_streaming => 1); +$standby->append_conf('postgresql.conf', 'hot_standby = on'); + +my $started = $standby->start(fail_ok => 1); +ok($started, 'the standby starts'); +if ($started) +{ + $primary->wait_for_catchup($standby); + is( $standby->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 on the standby'); +} +else +{ + my $log = slurp_file($standby->logfile); + diag("the standby did not start; the log has:\n" + . join('', grep { /invalid page|FATAL|WAL redo|consistent recovery/ } split(/^/m, $log))); +} + +done_testing(); -- 2.47.3