From d6babd31b984714950d91988ee2bea0b55131b72 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Thu, 12 Nov 2026 12:58:59 +0000 Subject: [PATCH v8 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 | 1 + .../recovery/t/061_hash_torn_primary_page.pl | 226 ++++++++++++++++++ 5 files changed, 263 insertions(+), 11 deletions(-) create mode 100644 src/test/recovery/t/061_hash_torn_primary_page.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..785f2d820b1 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_hash_torn_primary_page.pl', ], }, } diff --git a/src/test/recovery/t/061_hash_torn_primary_page.pl b/src/test/recovery/t/061_hash_torn_primary_page.pl new file mode 100644 index 00000000000..3b71fb409ed --- /dev/null +++ b/src/test/recovery/t/061_hash_torn_primary_page.pl @@ -0,0 +1,226 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test redo of a hash VACUUM record that reads a torn primary bucket page, whose +# repairing full-page image comes later in the WAL, in a standby started from a +# base backup and in crash recovery. Data checksums must be on, so that the +# torn page is detected. + +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; + +# Tear a block of a relation file, as a torn write does: keep the first half of +# the block and overwrite the second half with random bytes, so that its +# checksum does not match. +sub tear_page +{ + my ($path, $pblk, $bs) = @_; + + 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 $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()'); + +# Copy the data directory of the running server, as a backup does. Remove what +# must not be in a backup, as pg_basebackup does: postmaster.pid, which would +# make a server started from the copy think that the primary's postmaster is +# still running, postmaster.opts, which only records how the primary was +# started, and the WAL segments, which are copied at an arbitrary moment, and +# which the standby gets by streaming, starting at the redo pointer of the +# backup. The server expects pg_wal to exist, so create it empty. +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'); + +# Read the Hash records written since the checkpoint. +my $end_lsn = + $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn()'); +$primary->safe_psql('postgres', 'SELECT pg_switch_wal()'); + +# The first and the last WAL segment of the range. +my $start_seg = + $primary->safe_psql('postgres', "SELECT pg_walfile_name('$redo')"); +my $end_seg = + $primary->safe_psql('postgres', "SELECT pg_walfile_name('$end_lsn')"); + +# Dump the Hash records from the redo pointer of the backup to the end of the +# WAL written so far. The output is parsed below. +my ($stdout, $stderr); +IPC::Run::run( + [ + 'pg_waldump', + '--path' => $primary->data_dir . '/pg_wal', + '--start' => $redo, + '--end' => $end_lsn, + '--rmgr' => 'Hash', + $start_seg, $end_seg, + ], + '>' => \$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 #1: rel \S+ blk (\d+)/) + { + $pblk = $1; + last; + } +} +ok(defined $pblk, 'premise: found the primary bucket page') + or diag $stdout, $stderr; + +# The DELETE for an overflow page has to come before the first full-page image +# of the primary 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, $stderr; + +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, $stderr; +if (defined $idx_delete && defined $idx_fpi) +{ + cmp_ok($idx_delete, '<', $idx_fpi, + 'premise: the DELETE comes before the image of the primary page'); +} + +# Tear the copy of the primary page in the backup. +tear_page("$backup_dir/$relpath", $pblk, $bs) if defined $pblk; + +# 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); + +############################################################################### +# A standby started 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, 'standby: 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', + 'standby: the hash index gives the right result'); + $standby->stop; +} +else +{ + my $log = slurp_file($standby->logfile); + diag("standby: the standby did not start; the log has:\n" + . join('', grep { /invalid page|FATAL|WAL redo|consistent recovery/ } split(/^/m, $log))); +} + +############################################################################### +# Crash recovery of the primary +############################################################################### + +# There was no checkpoint since the one of the backup, so crash recovery +# replays the same records from the same redo pointer. Tear the primary's own +# copy of the page. +$primary->stop('immediate'); +tear_page($primary->data_dir . '/' . $relpath, $pblk, $bs) if defined $pblk; + +$started = $primary->start(fail_ok => 1); +ok($started, 'crash recovery: the server starts'); +if ($started) +{ + is( $primary->safe_psql( + 'postgres', + q{SET enable_seqscan = off; SET enable_bitmapscan = off; + SELECT count(*) FROM t WHERE k = 1}), + '400', + 'crash recovery: the hash index gives the right result'); +} +else +{ + my $log = slurp_file($primary->logfile); + diag("crash recovery: the server did not start; the log has:\n" + . join('', grep { /invalid page|FATAL|WAL redo/ } split(/^/m, $log))); +} + +done_testing(); -- 2.55.0