Hi Daniel, > Online enabling and disabling of data checksums > > [...]
I noticed a little mistake: ``` /* * Await state transition to "on" in all backends. When done we know that * data data checksums are both written and verified in all backends. */ ``` The word "data" is repeated twice. Also there are inconsistencies in the way XLogCtlData->data_checksum_version, ControlFileData->data_checksum_version and certain variables are assigned. Sometimes a hardcoded 0 is used and sometimes PG_DATA_CHECKSUM_OFF. I suggest using values of the enum ChecksumStateType for readability / consistency. Here are corresponding patches. -- Best regards, Aleksander Alekseev
From 14b1fb7baba5775e20dc7f50c8659250d1c667b2 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Mon, 6 Apr 2026 17:11:28 +0300 Subject: [PATCH v1 2/2] Use PG_DATA_CHECKSUM_OFF instead of hardcoded zeroes This is more readable and also more consistent with the rest of the code. Author: Aleksander Alekseev <[email protected]> Discussion: https://postgr.es/m/E1w8lyI-002o2A-2f%40gemulon.postgresql.org --- src/backend/access/transam/xlog.c | 8 ++++---- src/backend/bootstrap/bootstrap.c | 2 +- src/bin/pg_checksums/pg_checksums.c | 4 ++-- src/bin/pg_combinebackup/pg_combinebackup.c | 4 ++-- src/bin/pg_upgrade/controldata.c | 10 +++++----- src/bin/pg_upgrade/file.c | 2 +- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index fea479afaa9..260fc801ce2 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4864,7 +4864,7 @@ SetDataChecksumsOff(void) SpinLockAcquire(&XLogCtl->info_lck); /* If data checksums are already disabled there is nothing to do */ - if (XLogCtl->data_checksum_version == 0) + if (XLogCtl->data_checksum_version == PG_DATA_CHECKSUM_OFF) { SpinLockRelease(&XLogCtl->info_lck); return; @@ -4931,7 +4931,7 @@ SetDataChecksumsOff(void) XLogChecksums(PG_DATA_CHECKSUM_OFF); SpinLockAcquire(&XLogCtl->info_lck); - XLogCtl->data_checksum_version = 0; + XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_OFF; SpinLockRelease(&XLogCtl->info_lck); barrier = EmitProcSignalBarrier(PROCSIGNAL_BARRIER_CHECKSUM_OFF); @@ -6605,7 +6605,7 @@ StartupXLOG(void) XLogChecksums(PG_DATA_CHECKSUM_OFF); SpinLockAcquire(&XLogCtl->info_lck); - XLogCtl->data_checksum_version = 0; + XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_OFF; SetLocalDataChecksumState(XLogCtl->data_checksum_version); SpinLockRelease(&XLogCtl->info_lck); @@ -6625,7 +6625,7 @@ StartupXLOG(void) XLogChecksums(PG_DATA_CHECKSUM_OFF); SpinLockAcquire(&XLogCtl->info_lck); - XLogCtl->data_checksum_version = 0; + XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_OFF; SetLocalDataChecksumState(XLogCtl->data_checksum_version); SpinLockRelease(&XLogCtl->info_lck); } diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 63378ab3d8c..a4af7bf8fad 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -241,7 +241,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only) pg_getopt_ctx optctx; int flag; char *userDoption = NULL; - uint32 bootstrap_data_checksum_version = 0; /* No checksum */ + uint32 bootstrap_data_checksum_version = PG_DATA_CHECKSUM_OFF; yyscan_t scanner; Assert(!IsUnderPostmaster); diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 2a38f1d688b..cfacd1300fc 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -589,7 +589,7 @@ main(int argc, char *argv[]) mode == PG_MODE_CHECK) pg_fatal("data checksums are not enabled in cluster"); - if (ControlFile->data_checksum_version == 0 && + if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_OFF && mode == PG_MODE_DISABLE) pg_fatal("data checksums are already disabled in cluster"); @@ -645,7 +645,7 @@ main(int argc, char *argv[]) if (mode == PG_MODE_ENABLE || mode == PG_MODE_DISABLE) { ControlFile->data_checksum_version = - (mode == PG_MODE_ENABLE) ? PG_DATA_CHECKSUM_VERSION : 0; + (mode == PG_MODE_ENABLE) ? PG_DATA_CHECKSUM_VERSION : PG_DATA_CHECKSUM_OFF; if (do_sync) { diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index ac7eb0940d5..d13bf63eb1e 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -615,7 +615,7 @@ check_control_files(int n_backups, char **backup_dirs) { int i; uint64 system_identifier = 0; /* placate compiler */ - uint32 data_checksum_version = 0; /* placate compiler */ + uint32 data_checksum_version = PG_DATA_CHECKSUM_OFF; /* placate compiler */ bool data_checksum_mismatch = false; /* Try to read each control file in turn, last to first. */ @@ -652,7 +652,7 @@ check_control_files(int n_backups, char **backup_dirs) */ if (i == n_backups - 1) data_checksum_version = control_file->data_checksum_version; - else if (data_checksum_version != 0 && + else if (data_checksum_version != PG_DATA_CHECKSUM_OFF && data_checksum_version != control_file->data_checksum_version) data_checksum_mismatch = true; diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index 79053d22dcc..e18687226ae 100644 --- a/src/bin/pg_upgrade/controldata.c +++ b/src/bin/pg_upgrade/controldata.c @@ -206,7 +206,7 @@ get_control_data(ClusterInfo *cluster) /* Only in <= 9.2 */ if (GET_MAJOR_VERSION(cluster->major_version) <= 902) { - cluster->controldata.data_checksum_version = 0; + cluster->controldata.data_checksum_version = PG_DATA_CHECKSUM_OFF; got_data_checksum_version = true; } @@ -749,11 +749,11 @@ check_control_data(ControlData *oldctrl, * We might eventually allow upgrades from checksum to no-checksum * clusters. */ - if (oldctrl->data_checksum_version == 0 && - newctrl->data_checksum_version != 0) + if (oldctrl->data_checksum_version == PG_DATA_CHECKSUM_OFF && + newctrl->data_checksum_version != PG_DATA_CHECKSUM_OFF) pg_fatal("old cluster does not use data checksums but the new one does"); - else if (oldctrl->data_checksum_version != 0 && - newctrl->data_checksum_version == 0) + else if (oldctrl->data_checksum_version != PG_DATA_CHECKSUM_OFF && + newctrl->data_checksum_version == PG_DATA_CHECKSUM_OFF) pg_fatal("old cluster uses data checksums but the new one does not"); else if (oldctrl->data_checksum_version != newctrl->data_checksum_version) pg_fatal("old and new cluster pg_controldata checksum versions do not match"); diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index 4692e896326..5b276008614 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -331,7 +331,7 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile, break; /* Set new checksum for visibility map page, if enabled */ - if (new_cluster.controldata.data_checksum_version != 0) + if (new_cluster.controldata.data_checksum_version != PG_DATA_CHECKSUM_OFF) ((PageHeader) new_vmbuf.data)->pd_checksum = pg_checksum_page(new_vmbuf.data, new_blkno); -- 2.43.0
From 071cc93a34deebfab5cff6dbd26913d74f810166 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Mon, 6 Apr 2026 16:36:25 +0300 Subject: [PATCH v1 1/2] Fix doubled word in a comment introduced by commit f19c0eccae96 Author: Aleksander Alekseev <[email protected]> Discussion: https://postgr.es/m/E1w8lyI-002o2A-2f%40gemulon.postgresql.org --- src/backend/access/transam/xlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index b82af9a85c0..fea479afaa9 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4836,7 +4836,7 @@ SetDataChecksumsOn(void) /* * Await state transition to "on" in all backends. When done we know that - * data data checksums are both written and verified in all backends. + * data checksums are both written and verified in all backends. */ WaitForProcSignalBarrier(barrier); } -- 2.43.0
