Hi,
Am Mittwoch, den 26.12.2018, 19:43 +0100 schrieb Fabien COELHO:
> > It adds an (now mandatory) --action parameter that takes either verify,
> > enable or disable as argument.
>
> I'd rather have explicit switches for verify, enable & disable, and verify
> would be the default if none is provided.
I changed that to the switches -c/--verify (-c for check as -v is taken,
should it be --check as well? I personally like verify better),
-d/--disable and -e/--enable.
> About the patch: applies, compiles, "make check" ok.
>
> There is no documentation.
Yeah, I'll write that once the CLI is settled.
> In "scan_file", I would open RW only for enable, but keep RO for verify.
OK, I've changed that.
> Also, the full page is rewritten... would it make sense to only overwrite
> the checksum part itself?
So just writing the page header? I find that a bit scary and don't
expect much speedup as the OS would write the whole block anyway I
guess? I haven't touched that yet.
> It seems that the control file is unlinked and then rewritten. If the
> rewritting fails, or the command is interrupted, the user has a problem.
>
> Could the control file be simply opened RW? Else, I would suggest to
> rename (eg add .tmp), write the new one, then unlink the old one, so that
> recovering the old state in case of problem is possible.
I have mostly taken the pg_rewind code here; if there was a function
that allowed for safe offline changes of the control file, I'd be happy
to use it but I don't think it should be this patch to invent that.
In any case, I have removed the unlink() now (not sure where that came
from), and changed it to open(O_WRONLY) same as in Michael's code and
pg_rewind.
V2 attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@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
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index cc6ebb9df0..3ecce3da32 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-2019, 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,31 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+typedef enum
+{
+ 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_VERIFY;
+
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 +96,77 @@ 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);
+
+ fd = open(ControlFilePath, O_WRONLY | O_CREAT | PG_BINARY,
+ pg_file_create_mode);
+ if (fd < 0)
+ {
+ fprintf(stderr, _("%s: could not open 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 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 +174,14 @@ 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);
+
+ if (action == PG_ACTION_VERIFY)
+ f = open(fn, O_RDONLY | PG_BINARY, 0);
+ else
+ 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,7 +353,10 @@ int
main(int argc, char *argv[])
{
static struct option long_options[] = {
+ {"verify", no_argument, NULL, 'c'},
{"pgdata", required_argument, NULL, 'D'},
+ {"disable", no_argument, NULL, 'd'},
+ {"enable", no_argument, NULL, 'e'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,10 +384,19 @@ 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, "cD:der:v", long_options, &option_index)) != -1)
{
switch (c)
{
+ case 'c':
+ action = PG_ACTION_VERIFY;
+ break;
+ case 'd':
+ action = PG_ACTION_DISABLE;
+ break;
+ case 'e':
+ action = PG_ACTION_ENABLE;
+ break;
case 'v':
verbose = true;
break;
@@ -282,6 +417,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 +458,16 @@ main(int argc, char *argv[])
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 +479,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..af20c60445 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 => 59;
# 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', '-c', '-D', $pgdata, '-r',
+ $relfilenode_corrupted],
"succeeds for single relfilenode on tablespace $tablespace with offline cluster");
# Time to create some corruption
@@ -49,15 +49,15 @@ 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', '-c', '-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', '-c', '-D', $pgdata],
1,
[qr/Bad checksums:.*1/],
[qr/checksum verification failed/],
@@ -67,22 +67,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', '-c', '-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 +100,49 @@ 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', '-e', '-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');
+
+# Disable checksums again
+command_ok(['pg_verify_checksums', '-d', '-D', $pgdata],
+ "checksums successfully disabled in cluster");
+
+# Control file should know that checksums are disabled.
+command_like(['pg_controldata', $pgdata],
+ qr/Data page checksum version:.*0/,
+ 'checksums disabled in control file');
+
+# Enable checksums again with long option
+command_ok(['pg_verify_checksums', '--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', '-c', '-D', $pgdata],
"succeeds with offline cluster");
+# Checksums are verified if no other arguments are specified
+command_ok(['pg_verify_checksums', '-D', $pgdata],
+ "verifies checksums as default action");
+
+# Specific relation files cannot be requested when action is disable
+command_fails(['pg_verify_checksums', '-d', '-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', '-c', '-D', $pgdata],
"fails with online cluster");
# Check corruption of table on default tablespace.
@@ -133,7 +169,7 @@ 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', '-c', '-D', $pgdata],
1,
[qr/^$/],
[qr/could not read block 0 in file.*$file\":/],
