From d28fbfe399fc943e634ffcfe48ac59927cb80a04 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Sat, 25 Jul 2026 22:10:09 +0000 Subject: [PATCH] basebackup: do not verify checksums on pages from before enabling Enabling data checksums in a running cluster changes the cluster state to "on" before the checkpoint which flushes the pages the worker rewrote. A base backup which started before that transition absorbs the barrier mid-run and starts verifying pages whose on-disk copies legitimately lack checksums, and which carry LSNs older than the backup start so the LSN check does not skip them either. The backup fails with bogus corruption warnings and inflates the checksum failure counters. Backups from a standby have an even larger window since nothing forces a restartpoint after the state change. Record whether the checkpoint the backup starts from carried the fully enabled state, and verify checksums only if it did: such a checkpoint guarantees that every page flushed before it has a checksum written. The per-page state checks remain, they handle checksums being disabled while the backup runs. The test needs a shared buffer pool large enough to keep the rewritten pages dirty until the final checkpoint, otherwise they are written out with checksums before the backup reads them. --- src/backend/access/transam/xlog.c | 10 ++ src/backend/backup/basebackup.c | 19 +++- src/include/access/xlogbackup.h | 2 + src/test/modules/test_checksums/meson.build | 1 + .../test_checksums/t/010_backup_straddle.pl | 105 ++++++++++++++++++ 5 files changed, 132 insertions(+), 5 deletions(-) create mode 100644 src/test/modules/test_checksums/t/010_backup_straddle.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index f8b939853e9..8961ed2e7b7 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4844,6 +4844,8 @@ SetDataChecksumsOn(void) MyProc->delayChkptFlags &= ~DELAY_CHKPT_START; END_CRIT_SECTION(); + INJECTION_POINT("datachecksums-on-before-checkpoint", NULL); + RequestCheckpoint(CHECKPOINT_FORCE | CHECKPOINT_WAIT | CHECKPOINT_FAST); WaitForProcSignalBarrier(barrier); } @@ -9567,11 +9569,19 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces, * its REDO pointer. The oldest point in WAL that would be needed * to restore starting from the checkpoint is precisely the REDO * pointer. + * + * Also record whether that checkpoint had data checksums fully + * enabled. Enabling completes before the checkpoint which + * flushes the rewritten pages, so otherwise pages on disk can + * legitimately lack checksums even though their LSN predates the + * backup start. */ LWLockAcquire(ControlFileLock, LW_SHARED); state->checkpointloc = ControlFile->checkPoint; state->startpoint = ControlFile->checkPointCopy.redo; state->starttli = ControlFile->checkPointCopy.ThisTimeLineID; + state->checksums_on = (ControlFile->checkPointCopy.dataChecksumState == + PG_DATA_CHECKSUM_VERSION); checkpointfpw = ControlFile->checkPointCopy.fullPageWrites; LWLockRelease(ControlFileLock); diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index fe5ce23aaba..9c47c7e7b3b 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -134,6 +134,13 @@ static long long int total_checksum_failures; /* Do not verify checksums. */ static bool noverify_checksums = false; +/* + * Did the checkpoint this backup starts from have data checksums fully + * enabled? If not, pages older than the backup start can legitimately lack + * checksums, so nothing is verified for the duration of the backup. + */ +static bool checksums_on_at_start = false; + /* * Definition of one element part of an exclusion list, used for paths part * of checksum validation or base backups. "name" is the name of the file @@ -277,6 +284,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink, state.startptr = backup_state->startpoint; state.starttli = backup_state->starttli; + checksums_on_at_start = backup_state->checksums_on; /* * Once do_pg_backup_start has been called, ensure that any failure causes @@ -1609,13 +1617,14 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename, Assert((sink->bbs_buffer_length % BLCKSZ) == 0); /* - * If we weren't told not to verify checksums, and if checksums are - * enabled for this cluster, and if this is a relation file, then verify - * the checksum. We cannot at this point check if checksums are enabled - * or disabled as that might change, thus we check at each point where we + * If we weren't told not to verify checksums, and if checksums were + * enabled as of the checkpoint this backup started from, and if this is a + * relation file, then verify the checksum. Checksums can still be + * disabled while the backup runs, thus we check at each point where we * could be validating a checksum. */ - if (!noverify_checksums && RelFileNumberIsValid(relfilenumber)) + if (!noverify_checksums && checksums_on_at_start && + RelFileNumberIsValid(relfilenumber)) verify_checksum = true; /* diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h index 2cc2f85d9f0..4d448fcaf10 100644 --- a/src/include/access/xlogbackup.h +++ b/src/include/access/xlogbackup.h @@ -28,6 +28,8 @@ typedef struct BackupState XLogRecPtr checkpointloc; /* last checkpoint location */ pg_time_t starttime; /* backup start time */ bool started_in_recovery; /* backup started in recovery? */ + bool checksums_on; /* data checksums fully enabled as of the + * starting checkpoint? */ XLogRecPtr istartpoint; /* incremental based on backup at this LSN */ TimeLineID istarttli; /* incremental based on backup on this TLI */ diff --git a/src/test/modules/test_checksums/meson.build b/src/test/modules/test_checksums/meson.build index 9b1421a9b91..fc9f13fb848 100644 --- a/src/test/modules/test_checksums/meson.build +++ b/src/test/modules/test_checksums/meson.build @@ -33,6 +33,7 @@ tests += { 't/007_pgbench_standby.pl', 't/008_pitr.pl', 't/009_fpi.pl', + 't/010_backup_straddle.pl', ], }, } diff --git a/src/test/modules/test_checksums/t/010_backup_straddle.pl b/src/test/modules/test_checksums/t/010_backup_straddle.pl new file mode 100644 index 00000000000..6471c3507a9 --- /dev/null +++ b/src/test/modules/test_checksums/t/010_backup_straddle.pl @@ -0,0 +1,105 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test a base backup which is running when enabling data checksums completes. +# The transition to "on" happens before the checkpoint which flushes the +# rewritten pages, so such a backup reads on-disk pages which legitimately +# lack checksums and carry LSNs older than the backup start. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use IPC::Run; + +use FindBin; +use lib $FindBin::RealBin; + +use DataChecksums::Utils; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('straddle_node'); +$node->init(no_data_checksums => 1, allows_streaming => 1); +# The pages rewritten while enabling must stay dirty in shared buffers until +# the final checkpoint, otherwise they reach disk with checksums on their own +# and nothing is left to misjudge. +$node->append_conf('postgresql.conf', 'shared_buffers = 128MB'); +$node->start; + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# Enough data to keep the throttled backup running while checksums are being +# enabled. The scan pulls the table into shared buffers so that enabling +# doesn't read it through a ring buffer, which would write the pages back out. +$node->safe_psql('postgres', + "CREATE TABLE t AS SELECT generate_series(1,600000) AS a;"); +$node->safe_psql('postgres', "SELECT count(*) FROM t;"); +test_checksum_state($node, 'off'); + +# Hold the transition after the state changed to "on" but before the +# checkpoint which flushes the rewritten pages. +$node->safe_psql('postgres', + "SELECT injection_points_attach('datachecksums-on-before-checkpoint','wait');" +); + +my $backupdir = $node->backup_dir . '/straddle'; +my ($out, $err) = ('', ''); +my $backup = IPC::Run::start( + [ + 'pg_basebackup', '-D', $backupdir, + '--wal-method=none', '--no-sync', + '--checkpoint=fast', '--max-rate=4096', + '-d', $node->connstr('postgres') + ], + '>', \$out, '2>', \$err, + IPC::Run::timeout(180)); + +$node->poll_query_until('postgres', + "SELECT count(*) > 0 FROM pg_catalog.pg_stat_activity " + . "WHERE backend_type = 'walsender' AND state = 'active';"); + +enable_data_checksums($node); +$node->wait_for_event('datachecksums launcher', + 'datachecksums-on-before-checkpoint'); + +# The backup must still be sending files, otherwise it never sees the state +# change and the test is pointless. +my $running = $node->safe_psql('postgres', + "SELECT count(*) FROM pg_catalog.pg_stat_activity " + . "WHERE backend_type = 'walsender';"); +is($running, '1', 'backup still running when checksums were enabled'); + +ok($backup->finish, 'backup straddling enable completion succeeds') + or diag("stderr: $err"); + +$node->safe_psql('postgres', + "SELECT injection_points_wakeup('datachecksums-on-before-checkpoint');"); +$node->safe_psql('postgres', + "SELECT injection_points_detach('datachecksums-on-before-checkpoint');"); + +wait_for_checksum_state($node, 'on'); +$node->poll_query_until('postgres', + "SELECT count(*) = 0 FROM pg_catalog.pg_stat_activity " + . "WHERE backend_type = 'datachecksums launcher';"); + +my $result = $node->safe_psql('postgres', + "SELECT coalesce(sum(checksum_failures), 0) FROM pg_catalog.pg_stat_database;" +); +is($result, '0', 'no spurious checksum failures reported'); + +# A backup started once enabling has completed must verify, and pass +$node->command_ok( + [ + 'pg_basebackup', '-D', $node->backup_dir . '/after', + '--wal-method=none', '--no-sync', '--checkpoint=fast' + ], + 'backup after enable completion succeeds'); + +$node->stop; +done_testing(); -- 2.54.0