[Michael Banck <michael.banck@credativ.de>]

Hi,

the attached patch adds offline enabling/disabling of checksums to
pg_verify_checksums. It is based on independent work both Michael
(Paquier) and me did earlier this year and takes changes from both, see
https://github.com/credativ/pg_checksums and
https://github.com/michaelpq/pg_plugins/tree/master/pg_checksums

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

This is basically meant as a stop-gap measure in case online activation
of checksums won't make it for v12, but maybe it is independently
useful?

Things I have not done so far:

1. Rename pg_verify_checksums to e.g. pg_checksums as it will no longer
only verify checksums.

2. Rename the scan_* functions (Michael renamed them to operate_file and
operate_directory but I am not sure it is worth it.

3. Once that patch is in, there would be a way to disable checksums so
there'd be a case to also change the initdb default to enabled, but that
required further discussion (and maybe another round of benchmarks).


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachments:

  [text/x-diff] offline-activation-of-checksums_V1.patch (15.7K, ../20181221201616.GD4974@nighthawk.caipicrew.dd-dns.de/2-offline-activation-of-checksums_V1.patch)
  download | inline diff:
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 6444fc9ca4..65d6195509 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -1,7 +1,7 @@
 /*
  * pg_verify_checksums
  *
- * Verifies page level checksums in an offline cluster
+ * Verifies/enables/disables page level checksums in an offline cluster
  *
  *	Copyright (c) 2010-2018, PostgreSQL Global Development Group
  *
@@ -13,15 +13,16 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "access/xlog_internal.h"
 #include "catalog/pg_control.h"
 #include "common/controldata_utils.h"
+#include "common/file_perm.h"
+#include "common/file_utils.h"
 #include "getopt_long.h"
 #include "pg_getopt.h"
 #include "storage/bufpage.h"
 #include "storage/checksum.h"
 #include "storage/checksum_impl.h"
-#include "storage/fd.h"
-
 
 static int64 files = 0;
 static int64 blocks = 0;
@@ -31,16 +32,32 @@ static ControlFileData *ControlFile;
 static char *only_relfilenode = NULL;
 static bool verbose = false;
 
+typedef enum
+{
+	PG_ACTION_NONE,
+	PG_ACTION_DISABLE,
+	PG_ACTION_ENABLE,
+	PG_ACTION_VERIFY
+} ChecksumAction;
+
+/* Filename components */
+#define PG_TEMP_FILES_DIR "pgsql_tmp"
+#define PG_TEMP_FILE_PREFIX "pgsql_tmp"
+
+static ChecksumAction action = PG_ACTION_NONE;
+
 static const char *progname;
 
 static void
 usage(void)
 {
-	printf(_("%s verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
+	printf(_("%s enables/disables/verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
 	printf(_("Usage:\n"));
 	printf(_("  %s [OPTION]... [DATADIR]\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_(" [-D, --pgdata=]DATADIR  data directory\n"));
+	printf(_("  -A, --action   action to take on the cluster, can be set as\n"));
+	printf(_("                 \"verify\", \"enable\" and \"disable\"\n"));
 	printf(_("  -v, --verbose          output verbose messages\n"));
 	printf(_("  -r RELFILENODE         check only relation with specified relfilenode\n"));
 	printf(_("  -V, --version          output version information, then exit\n"));
@@ -80,6 +97,80 @@ skipfile(const char *fn)
 }
 
 static void
+updateControlFile(char *DataDir, ControlFileData *ControlFile)
+{
+	int			fd;
+	char		buffer[PG_CONTROL_FILE_SIZE];
+	char		ControlFilePath[MAXPGPATH];
+
+	Assert(action == PG_ACTION_ENABLE ||
+		   action == PG_ACTION_DISABLE);
+
+	/*
+	 * For good luck, apply the same static assertions as in backend's
+	 * WriteControlFile().
+	 */
+#if PG_VERSION_NUM >= 100000
+	StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_MAX_SAFE_SIZE,
+					 "pg_control is too large for atomic disk writes");
+#endif
+	StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_FILE_SIZE,
+					 "sizeof(ControlFileData) exceeds PG_CONTROL_FILE_SIZE");
+
+	/* Recalculate CRC of control file */
+	INIT_CRC32C(ControlFile->crc);
+	COMP_CRC32C(ControlFile->crc,
+				(char *) ControlFile,
+				offsetof(ControlFileData, crc));
+	FIN_CRC32C(ControlFile->crc);
+
+	/*
+	 * Write out PG_CONTROL_FILE_SIZE bytes into pg_control by zero-padding
+	 * the excess over sizeof(ControlFileData), to avoid premature EOF related
+	 * errors when reading it.
+	 */
+	memset(buffer, 0, PG_CONTROL_FILE_SIZE);
+	memcpy(buffer, ControlFile, sizeof(ControlFileData));
+
+	snprintf(ControlFilePath, sizeof(ControlFilePath), "%s/%s", DataDir, XLOG_CONTROL_FILE);
+
+	unlink(ControlFilePath);
+
+	fd = open(ControlFilePath,
+			  O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
+			  pg_file_create_mode);
+	if (fd < 0)
+	{
+		fprintf(stderr, _("%s: could not open pg_control file: %s\n"),
+				progname, strerror(errno));
+		exit(1);
+	}
+
+	errno = 0;
+	if (write(fd, buffer, PG_CONTROL_FILE_SIZE) != PG_CONTROL_FILE_SIZE)
+	{
+		/* if write didn't set errno, assume problem is no disk space */
+		if (errno == 0)
+			errno = ENOSPC;
+		fprintf(stderr, _("%s: could not write pg_control file: %s\n"),
+				progname, strerror(errno));
+		exit(1);
+	}
+
+	if (fsync(fd) != 0)
+	{
+		fprintf(stderr, _("%s: fsync error: %s\n"), progname, strerror(errno));
+		exit(1);
+	}
+
+	if (close(fd) < 0)
+	{
+		fprintf(stderr, _("%s: could not close control file: %s\n"), progname, strerror(errno));
+		exit(1);
+	}
+}
+
+static void
 scan_file(const char *fn, BlockNumber segmentno)
 {
 	PGAlignedBlock buf;
@@ -87,7 +178,10 @@ scan_file(const char *fn, BlockNumber segmentno)
 	int			f;
 	BlockNumber blockno;
 
-	f = open(fn, O_RDONLY | PG_BINARY, 0);
+	Assert(action == PG_ACTION_ENABLE ||
+		   action == PG_ACTION_VERIFY);
+
+	f = open(fn, O_RDWR | PG_BINARY, 0);
 	if (f < 0)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
@@ -117,18 +211,47 @@ scan_file(const char *fn, BlockNumber segmentno)
 			continue;
 
 		csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
-		if (csum != header->pd_checksum)
+		if (action == PG_ACTION_VERIFY)
 		{
-			if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
-				fprintf(stderr, _("%s: checksum verification failed in file \"%s\", block %u: calculated checksum %X but block contains %X\n"),
-						progname, fn, blockno, csum, header->pd_checksum);
-			badblocks++;
+			if (csum != header->pd_checksum)
+			{
+				if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
+					fprintf(stderr, _("%s: checksum verification failed in file \"%s\", block %u: calculated checksum %X but block contains %X\n"),
+							progname, fn, blockno, csum, header->pd_checksum);
+				badblocks++;
+			}
+		}
+		else if (action == PG_ACTION_ENABLE)
+		{
+			/* Set checksum in page header */
+			header->pd_checksum = csum;
+
+			/* Seek back to beginning of block */
+			if (lseek(f, -BLCKSZ, SEEK_CUR) < 0)
+			{
+				fprintf(stderr, _("%s: seek failed for block %d in file \"%s\": %s\n"), progname, blockno, fn, strerror(errno));
+				exit(1);
+			}
+
+			/* Write block with checksum */
+			if (write(f, buf.data, BLCKSZ) != BLCKSZ)
+			{
+				fprintf(stderr, "%s: could not update checksum of block %d in file \"%s\": %s\n",
+						progname, blockno, fn, strerror(errno));
+				exit(1);
+			}
 		}
 	}
 
 	if (verbose)
-		fprintf(stderr,
-				_("%s: checksums verified in file \"%s\"\n"), progname, fn);
+	{
+		if (action == PG_ACTION_VERIFY)
+			fprintf(stderr,
+					_("%s: checksums verified in file \"%s\"\n"), progname, fn);
+		if (action == PG_ACTION_ENABLE)
+			fprintf(stderr,
+					_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
+	}
 
 	close(f);
 }
@@ -230,6 +353,7 @@ int
 main(int argc, char *argv[])
 {
 	static struct option long_options[] = {
+		{"action", required_argument, NULL, 'A'},
 		{"pgdata", required_argument, NULL, 'D'},
 		{"verbose", no_argument, NULL, 'v'},
 		{NULL, 0, NULL, 0}
@@ -258,10 +382,31 @@ main(int argc, char *argv[])
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+	while ((c = getopt_long(argc, argv, "A:D:r:v", long_options, &option_index)) != -1)
 	{
 		switch (c)
 		{
+			case 'A':
+				/* Check for redundant options */
+				if (action != PG_ACTION_NONE)
+				{
+					fprintf(stderr, _("%s: action already specified.\n"), progname);
+					exit(1);
+				}
+
+				if (strcmp(optarg, "verify") == 0)
+					action = PG_ACTION_VERIFY;
+				else if (strcmp(optarg, "disable") == 0)
+					action = PG_ACTION_DISABLE;
+				else if (strcmp(optarg, "enable") == 0)
+					action = PG_ACTION_ENABLE;
+				else
+				{
+					fprintf(stderr, _("%s: incorrect action \"%s\" specified.\n"),
+									progname, optarg);
+					exit(1);
+				}
+				break;
 			case 'v':
 				verbose = true;
 				break;
@@ -282,6 +427,21 @@ main(int argc, char *argv[])
 		}
 	}
 
+	/*
+	 * Don't allow pg_checksums to be run as root, to avoid overwriting the
+	 * ownership of files in the data directory. We need only check for root
+	 * -- any other user won't have sufficient permissions to modify files in
+	 * the data directory.  This does not matter for the "verify" mode, but
+	 * let's be consistent.
+	 */
+#ifndef WIN32
+	if (geteuid() == 0)
+	{
+		fprintf(stderr, _("%s: cannot be executed by \"root\"\n"), progname);
+		exit(1);
+	}
+#endif
+
 	if (DataDir == NULL)
 	{
 		if (optind < argc)
@@ -308,6 +468,25 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	/* Complain if no action has been requested */
+	if (action == PG_ACTION_NONE)
+	{
+		fprintf(stderr, _("%s: no action specified\n"), progname);
+		fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+						progname);
+		exit(1);
+	}
+
+	/* Relfilenode checking only works in verify mode */
+	if (action != PG_ACTION_VERIFY &&
+		only_relfilenode)
+	{
+		fprintf(stderr, _("%s: relfilenode option only possible with verify action\n"), progname);
+		fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+						progname);
+		exit(1);
+	}
+
 	/* Check if cluster is running */
 	ControlFile = get_controlfile(DataDir, progname, &crc_ok);
 	if (!crc_ok)
@@ -319,29 +498,74 @@ main(int argc, char *argv[])
 	if (ControlFile->state != DB_SHUTDOWNED &&
 		ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
 	{
-		fprintf(stderr, _("%s: cluster must be shut down to verify checksums\n"), progname);
+		fprintf(stderr, _("%s: cluster must be shut down\n"), progname);
 		exit(1);
 	}
 
-	if (ControlFile->data_checksum_version == 0)
+	if (ControlFile->data_checksum_version == 0 &&
+		action == PG_ACTION_VERIFY)
 	{
 		fprintf(stderr, _("%s: data checksums are not enabled in cluster\n"), progname);
 		exit(1);
 	}
+	if (ControlFile->data_checksum_version == 0 &&
+		action == PG_ACTION_DISABLE)
+	{
+		fprintf(stderr, _("%s: data checksums are already disabled in cluster.\n"), progname);
+		exit(1);
+	}
+	if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION &&
+		action == PG_ACTION_ENABLE)
+	{
+		fprintf(stderr, _("%s: data checksums are already enabled in cluster.\n"), progname);
+		exit(1);
+	}
 
-	/* Scan all files */
+	/*
+	 * When disabling data checksums, only update the control file and call it
+	 * a day.
+	 */
+	if (action == PG_ACTION_DISABLE)
+	{
+		ControlFile->data_checksum_version = 0;
+		updateControlFile(DataDir, ControlFile);
+		fsync_pgdata(DataDir, progname, PG_VERSION_NUM);
+		if (verbose)
+			printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
+		printf(_("Checksums disabled in cluster\n"));
+		return 0;
+	}
+
+	/* Operate on all files */
 	scan_directory(DataDir, "global");
 	scan_directory(DataDir, "base");
 	scan_directory(DataDir, "pg_tblspc");
 
-	printf(_("Checksum scan completed\n"));
-	printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
+	printf(_("Checksum operation completed\n"));
 	printf(_("Files scanned:  %s\n"), psprintf(INT64_FORMAT, files));
 	printf(_("Blocks scanned: %s\n"), psprintf(INT64_FORMAT, blocks));
-	printf(_("Bad checksums:  %s\n"), psprintf(INT64_FORMAT, badblocks));
+	if (action == PG_ACTION_VERIFY)
+	{
+		printf(_("Bad checksums:  %s\n"), psprintf(INT64_FORMAT, badblocks));
+		printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
 
-	if (badblocks > 0)
+		if (badblocks > 0)
 		return 1;
+	}
+
+	/*
+	 * When enabling checksums, wait until the end the operation has completed
+	 * to do the switch.
+	 */
+	if (action == PG_ACTION_ENABLE)
+	{
+		ControlFile->data_checksum_version = PG_DATA_CHECKSUM_VERSION;
+		updateControlFile(DataDir, ControlFile);
+		fsync_pgdata(DataDir, progname, PG_VERSION_NUM);
+		if (verbose)
+			printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
+		printf(_("Checksums enabled in cluster\n"));
+	}
 
 	return 0;
 }
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
index 5250b5a728..0dba764a23 100644
--- a/src/bin/pg_verify_checksums/t/002_actions.pl
+++ b/src/bin/pg_verify_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 45;
+use Test::More tests => 50;
 
 
 # Utility routine to create and check a table with corrupted checksums
@@ -38,8 +38,8 @@ sub check_relation_corruption
 
 	# Checksums are correct for single relfilenode as the table is not
 	# corrupted yet.
-	command_ok(['pg_verify_checksums',  '-D', $pgdata,
-		'-r', $relfilenode_corrupted],
+	command_ok(['pg_verify_checksums',  '-A', 'verify', '-D', $pgdata, '-r',
+			   $relfilenode_corrupted],
 		"succeeds for single relfilenode on tablespace $tablespace with offline cluster");
 
 	# Time to create some corruption
@@ -49,15 +49,16 @@ sub check_relation_corruption
 	close $file;
 
 	# Checksum checks on single relfilenode fail
-	$node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata, '-r',
-								$relfilenode_corrupted],
+	$node->command_checks_all([ 'pg_verify_checksums', '-A', 'verify', '-D',
+							  $pgdata, '-r', $relfilenode_corrupted],
 							  1,
 							  [qr/Bad checksums:.*1/],
 							  [qr/checksum verification failed/],
 							  "fails with corrupted data for single relfilenode on tablespace $tablespace");
 
 	# Global checksum checks fail as well
-	$node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata],
+	$node->command_checks_all([ 'pg_verify_checksums', '-A', 'verify', '-D',
+							  $pgdata],
 							  1,
 							  [qr/Bad checksums:.*1/],
 							  [qr/checksum verification failed/],
@@ -67,22 +68,22 @@ sub check_relation_corruption
 	$node->start;
 	$node->safe_psql('postgres', "DROP TABLE $table;");
 	$node->stop;
-	$node->command_ok(['pg_verify_checksums', '-D', $pgdata],
+	$node->command_ok(['pg_verify_checksums', '-A', 'verify', '-D', $pgdata],
 	        "succeeds again after table drop on tablespace $tablespace");
 
 	$node->start;
 	return;
 }
 
-# Initialize node with checksums enabled.
+# Initialize node with checksums disabled.
 my $node = get_new_node('node_checksum');
-$node->init(extra => ['--data-checksums']);
+$node->init();
 my $pgdata = $node->data_dir;
 
-# Control file should know that checksums are enabled.
+# Control file should know that checksums are disabled.
 command_like(['pg_controldata', $pgdata],
-	     qr/Data page checksum version:.*1/,
-		 'checksums enabled in control file');
+	     qr/Data page checksum version:.*0/,
+		 'checksums disabled in control file');
 
 # These are correct but empty files, so they should pass through.
 append_to_file "$pgdata/global/99999", "";
@@ -100,13 +101,27 @@ append_to_file "$pgdata/global/pgsql_tmp_123", "foo";
 mkdir "$pgdata/global/pgsql_tmp";
 append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
 
+# Enable checksums
+command_ok(['pg_verify_checksums', '-A', 'enable', '-D', $pgdata],
+		   "checksums successfully enabled in cluster");
+
+# Control file should know that checksums are enabled.
+command_like(['pg_controldata', $pgdata],
+	     qr/Data page checksum version:.*1/,
+		 'checksums enabled in control file');
+
 # Checksums pass on a newly-created cluster
-command_ok(['pg_verify_checksums',  '-D', $pgdata],
+command_ok(['pg_verify_checksums', '-A', 'verify', '-D', $pgdata],
 		   "succeeds with offline cluster");
 
+# Specific relation files cannot be requested when action is disable
+command_fails(['pg_verify_checksums', '-A', 'disable', '-r', '1234', '-D',
+			  $pgdata],
+			  "fails when relfilnodes are requested and action is not verify");
+
 # Checks cannot happen with an online cluster
 $node->start;
-command_fails(['pg_verify_checksums',  '-D', $pgdata],
+command_fails(['pg_verify_checksums', '-A', 'verify', '-D', $pgdata],
 			  "fails with online cluster");
 
 # Check corruption of table on default tablespace.
@@ -133,7 +148,8 @@ sub fail_corrupt
 	my $file_name = "$pgdata/global/$file";
 	append_to_file $file_name, "foo";
 
-	$node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata],
+	$node->command_checks_all([ 'pg_verify_checksums', '-A', 'verify', '-D',
+						  $pgdata],
 						  1,
 						  [qr/^$/],
 						  [qr/could not read block 0 in file.*$file\":/],