Date: Fri, 28 Aug 2026 05:33:34 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Zsolt Parragi <zsolt.parragi@percona.com>
Subject: Re: Offline data checksum changes can cause incorrect checksum state
 on standbys
Message-ID: <apEdrmkoNT6R9XyJ@bdtpg>
References: <anwm6UPxoVS41QA2@bdtpg>
 <1C2BC974-44FA-487C-8A3B-8135A316A89B@yesql.se>
 <an8z8CJMa46FIxxn@bdtpg>
 <188A1307-1A92-45CA-9DBE-FB0962D3756E@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <188A1307-1A92-45CA-9DBE-FB0962D3756E@yesql.se>
Archived-At: <https://www.postgresql.org/message-id/apEdrmkoNT6R9XyJ%40bdtpg>
Precedence: bulk

Hi,

On Wed, Aug 26, 2026 at 10:37:09PM +0200, Daniel Gustafsson wrote:
> > On 14 Aug 2026, at 17:27, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> 
> > Looking forward to seeing your and Zsolt's proposals.
> 
> This has now been worked on quite extensively by Zsolt, myself and Tomas Vondra
> and a number of patchrevisions have been created and rewritten.

Thanks for looking at it!

> The regression in a correctly done offline change is due to combining
> pg_checksums which rewrite data on risk without WAL logging (or any logging at
> all) the transformation, with online checksums which WAL log the state change.
> With online checksums, the local state on the standby was overwritten during
> replay by the dataChecksumState in the checkpoint.

Agreed.

> The fix in 0001 is to not adopt the
> state change from the replay of checkpoints, only from XLOG2_CHECKSUMS records,
> and to alert the user with a log entry if the states mismatch.

That makes sense to me and matches the intent of my v1 for this regression: keep
offline changes local while still applying WAL online transitions, with the useful
addition of a warning on mismatch.

> The 0001 patch is the least invasive patch to solve the regression that either
> of us has managed to come up with, but it's still far from trivial.

Only looking at 0001 here, I've a few comments:

=== 1

@@ -9288,17 +9572,33 @@ xlog2_redo(XLogReaderState *record)

        SpinLockAcquire(&XLogCtl->info_lck);
        XLogCtl->data_checksum_version = state.new_checksum_state;
+       SetLocalDataChecksumState(state.new_checksum_state);
        SpinLockRelease(&XLogCtl->info_lck);

This applies every XLOG2_CHECKSUMS record encountered during recovery, even when
the same record was applied before.

For example, a standby can replay the final "on" record and then stop cleanly
without advancing its restartpoint beyond that record. If checksums are subsequently
disabled offline, the next startup begins from the older restartpoint and replays
the same on record again, overriding the offline disable.

=== 2

+    * checkPoint.dataChecksumState was sampled while holding the WAL insert
+    * locks, so it is the state in effect at the redo point. 
.
.
.
+   SpinLockAcquire(&XLogCtl->info_lck);
+   if (checkPoint.dataChecksumState == XLogCtl->data_checksum_version)
+       ControlFile->data_checksum_version = checkPoint.dataChecksumState;
+   SpinLockRelease(&XLogCtl->info_lck);

I’m not sure the "state in effect at the redo point" is always correct. There is
a window between inserting the checksum transition record and updating
XLogCtl->data_checksum_version.

XLOG2_CHECKSUMS(on) records the target state. The transition can therefore
proceed as follows:

1. The current shared state is inprogress-on.
2. XLogChecksums() inserts XLOG2_CHECKSUMS(on) and releases its WAL insertion
   lock.
3. Before the shared state is updated to on, the checkpoint still reads
   inprogress-on and inserts XLOG_CHECKPOINT_REDO.
4. The transition then updates the shared state to on.

The WAL order is then:

XLOG2_CHECKSUMS(on)
XLOG_CHECKPOINT_REDO(inprogress-on)

If the server crashes after the concurrent checkpoint from step 3 completes,
but before the enabling operation’s later checkpoint completes, recovery starts
from that redo point and does not replay the preceding on record. It can
therefore resolve inprogress-on back to off. The equality check above does
not repair this ordering.

=== 3

+   if ((haveBackupLabel || XLogRecPtrIsValid(ControlFile->backupStartPoint)) &&
+       !(XLogRecPtrIsValid(ControlFile->backupEndPoint) &&
+         ControlFile->backupEndRequired))
+   {
+       if (wasShutdown)
+           AdoptReplayedDataChecksumState(checkPoint.dataChecksumState);
+       else
+           adoptChecksumStateFromNextCheckpoint = true;
+   }

IIUC, this can overwrite the checksum state copied from the source during
pg_rewind with the state from the last common checkpoint.

For example, if the common checkpoint says off, but both source and target
were enabled offline after divergence, recovery adopts off. Since the
offline enable has no XLOG2_CHECKSUMS record, nothing restores on.

I have only looked at 0001 for this point, so I don't know whether one of the
following patches handles this case.

=== 4

+   /*
+    * Note the data checksum state the flush below starts under.  Replay runs
+    * concurrently and can change the state while the flush is in progress,
+    * in which case the flush covers pages written under both states; see
+    * where the state is persisted further down.
+    */
+   SpinLockAcquire(&XLogCtl->info_lck);
+   checksum_state = XLogCtl->data_checksum_version;
+   SpinLockRelease(&XLogCtl->info_lck);
+

    CheckPointGuts(lastCheckPoint.redo, flags);

+        * Persist only if the flush above ran under one state throughout; see
+        * CreateCheckPoint() for why.
+        */
+       SpinLockAcquire(&XLogCtl->info_lck);
+       if (checksum_state == XLogCtl->data_checksum_version)
+           ControlFile->data_checksum_version = checksum_state;
+       SpinLockRelease(&XLogCtl->info_lck);

I think comparing only data_checksum_version cannot detect a complete
on->off->on transition during CheckPointGuts(). The initial and final values
match even though the flush ran under multiple states.

FWIW, while v4-0001 may address other issues present in v1, v1 would avoid the
specific cases described in === 1 through === 3. Some parts of it may therefore
be worth considering here.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com