Re: basebackup: do not verify checksums on pages written before enabling checksums

Hi,

On Sat, Aug 15, 2026 at 10:58:53AM +0200, Daniel Gustafsson wrote:
> > On 15 Aug 2026, at 10:10, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
> >
> > That seems like a good idea, that would allow us to remove the static
> > variable. v2 attached based on this approach.

Thanks!

> I prefer this approach,

+1

A few comments:

=== 1

+ * record inserted or replayed, i.e. the last change of
+ * data_checksum_version. InvalidXLogRecPtr if the state hasn't changed
+ * since the server started.
+ */
+ XLogRecPtr lastChecksumChangeRecPtr;

I'm not sure that "since the server started" is enough for a backup taken from
a standby. XLogCtl is zeroed at startup, while the checksum state is restored
from pg_control. The standby can become available before replay reaches the
latest checksum transition, so verification could resume with
lastChecksumChangeRecPtr invalid.

I wonder if the transition LSN should survive a server restart?

=== 2

+GetLastChecksumChangeRecPtr(void)
+{
+ XLogRecPtr ptr;
+
+ SpinLockAcquire(&XLogCtl->info_lck);
+ ptr = XLogCtl->lastChecksumChangeRecPtr;
+ SpinLockRelease(&XLogCtl->info_lck);

Then:

+backup_checksums_verifiable(XLogRecPtr start_lsn)
+{
+ return DataChecksumsNeedVerify() &&
+ GetLastChecksumChangeRecPtr() <= start_lsn;

and:

@@ -1876,7 +1880,7 @@ read_file_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
* The data checksum state can change at any point, so we need to
* re-check before each page.
*/
- if (!DataChecksumsNeedVerify())
+ if (!backup_checksums_verifiable(sink->bbs_state->startptr))

and:

@@ -2021,7 +2046,7 @@ verify_page_checksum(Page page, XLogRecPtr start_lsn, BlockNumber blkno,
if (PageIsNew(page) || PageGetLSN(page) >= start_lsn)
return true;

- if (!DataChecksumsNeedVerify())
+ if (!backup_checksums_verifiable(start_lsn))

This means two acquisitions of the spinlock per verified page.

I think that an atomic would make more sense. XLogCtlData already uses atomics,
and lastChecksumChangeRecPtr does not need to be read consistently with any other
field.

Regards,

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