On Wed, Feb 27, 2019 at 07:59:31AM +0100, Fabien COELHO wrote: > Hallo Michael,
Okay, let's move on with these patches! > The renaming implies quite a few changes (eg in the documentation, > makefiles, tests) which warrants a review, so it should be a patch. Also, > ISTM that the renaming only make sense when adding the enable/disable > feature, so I'd say that it belongs to this patch. Opinions? I have worked on the last v4 sent by Michael B, finishing with the attached after review and addressed the last points raised by Fabien. The thing is that I have been rather unhappy with a couple of things in what was proposed, so I have finished by modifying quite a couple of areas. The patch set is now splitted as I think is suited for commit, with the refactoring and renaming being separated from the actual feature: - 0001 if a patch to refactor the routine for the control file update. I have made it backend-aware, and we ought to be careful with error handling, use of fds and such, something that v4 was not very careful about. - 0002 renames pg_verify_checksums to pg_checksums with a straight-forward switch. Docs as well as all references to pg_verify_checksums are updated. - 0003 adds the new options --check, --enable and --disable, with --check being the default as discussed. - 0004 adds a -N/--no-sync which I think is nice for consistency with other tools. That's also useful for the tests, and was not discussed until now on this thread. > Patch applies cleanly, compiles, global & local "make check" are ok. > > Doc: "enable, disable or verifies" -> "checks, enables or disables"? > Spurious space: "> ." -> ">.". > > As checksum are now checked, the doc could use "check" instead of "verify", > especially if there is a rename and the "verify" word disappears. That makes sense. I have fixed these, and simplified the docs a bit to have a more simple page. > I'd be less terse when documenting actions, eg: "Disable checksums" -> > "Disable checksums on cluster." The former is fine in my opinion. > Doc should state that checking is the default action, eg "Check checksums on > cluster. This is the default action." Check. > Help string could say that -c is the default action. There is a spurious > help line remaining from the previous "--action" implementation. Yeah, we should. Added. > open: I'm positively unsure about ?: priority over |, and probably not the > only one, so I'd add parentheses around the former. Yes, I agree that the current code is hard to decrypt. So reworked with a separate variable in scan_file, and added extra parenthesis in the part which updates the control file. > I'm at odds with the "postmaster.pid" check, which would not prevent an > issue if a cluster is started with "postmaster". I still think that the > enabling-in-progress should be stored in the cluster state. > > ISTM that the cluster read/update cycle should lock somehow the control file > being modified. However other commands do not seem to do something about it. I am still not on board for adding more complexity in this area, at least not for this stuff and for the purpose of this thread, because this can happen at various degrees for various configurations for ages and not only for checksums. Also, I think that we still have to see users complain about that. Here are some scenarios where this can happen: - A base backup partially written. pg_basebackup limits this risk but it could still be possible to see a case where partially-written data folder. And base backups are around for many years. - pg_rewind, and the tool is in the tree since 9.5, the tool is actually available on github since 9.3. > I do not think that enabling if already enabled or disabling or already > disable should exit(1), I think it is a no-op and should simply exit(0). We already issue exit(1) when attempting to verify checksums on a cluster where they are disabled. So I agree with Michael B's point of Issuing an error in such cases. I think also that this makes handling for operators easier. > About tests: I'd run a check on a disabled cluster to check that the command > fails because disabled. Makes sense. Added. We need a test also for the case of successive runs with --enable. Here are also some notes from my side. - There was no need to complicate the synopsis of the docs. - usage() included still references to --action and indentation was a bit too long at the top. - There were no tests for disabling checksums, so I have added some. - We should check that the combination of --enable and -r fails. - Tests use only long options, that's better for readability. - Improved comments in tests. - Better to check for "data_checksum_version > 0" if --enable is used. That's more portable long-term if more checksum versions are added. - The check on postmaster.pid is actually not necessary as we already know that the cluster has been shutdown cleanly per the state of the control file. I agree that there could be a small race condition here, and we could discuss that in a different thread if need be as such things could be improved for other frontend tools as well. For now I am taking the most simple approach. (Still need to indent the patches before commit, but that's a nit.) -- Michael
From 35b089e29f78704f40b0cbc1d2a912e1eb649869 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 11 Mar 2019 11:17:07 +0900 Subject: [PATCH 1/4] Refactor routine for update of control file This adds a new routine to src/common/ which is compatible with both the frontend and backend code able to update a control file's contents. This is now getting used only by pg_rewind, and some upcoming patches for offline checksums will make use of it. Author: Michael Banck, Michael Paquier Reviewed-by: Fabien Coelho Discussion: https://postgr.es/m/20181221201616.gd4...@nighthawk.caipicrew.dd-dns.de --- src/bin/pg_rewind/pg_rewind.c | 43 +----------- src/common/controldata_utils.c | 93 ++++++++++++++++++++++++++ src/include/common/controldata_utils.h | 6 +- 3 files changed, 100 insertions(+), 42 deletions(-) diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index aa753bb315..7f1d6bf48a 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -24,6 +24,7 @@ #include "access/xlog_internal.h" #include "catalog/catversion.h" #include "catalog/pg_control.h" +#include "common/controldata_utils.h" #include "common/file_perm.h" #include "common/file_utils.h" #include "common/restricted_token.h" @@ -37,7 +38,6 @@ static void createBackupLabel(XLogRecPtr startpoint, TimeLineID starttli, static void digestControlFile(ControlFileData *ControlFile, char *source, size_t size); -static void updateControlFile(ControlFileData *ControlFile); static void syncTargetDirectory(void); static void sanityChecks(void); static void findCommonAncestorTimeline(XLogRecPtr *recptr, int *tliIndex); @@ -377,7 +377,7 @@ main(int argc, char **argv) ControlFile_new.minRecoveryPoint = endrec; ControlFile_new.minRecoveryPointTLI = endtli; ControlFile_new.state = DB_IN_ARCHIVE_RECOVERY; - updateControlFile(&ControlFile_new); + update_controlfile(datadir_target, progname, &ControlFile_new); pg_log(PG_PROGRESS, "syncing target data directory\n"); syncTargetDirectory(); @@ -666,45 +666,6 @@ digestControlFile(ControlFileData *ControlFile, char *src, size_t size) checkControlFile(ControlFile); } -/* - * Update the target's control file. - */ -static void -updateControlFile(ControlFileData *ControlFile) -{ - char buffer[PG_CONTROL_FILE_SIZE]; - - /* - * For good luck, apply the same static assertions as in backend's - * WriteControlFile(). - */ - StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_MAX_SAFE_SIZE, - "pg_control is too large for atomic disk writes"); - 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)); - - open_target_file("global/pg_control", false); - - write_target_range(buffer, 0, PG_CONTROL_FILE_SIZE); - - close_target_file(); -} - /* * Sync target data directory to ensure that modifications are safely on disk. * diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c index 6289a4343a..1e44f36765 100644 --- a/src/common/controldata_utils.c +++ b/src/common/controldata_utils.c @@ -24,8 +24,10 @@ #include <sys/stat.h> #include <fcntl.h> +#include "access/xlog_internal.h" #include "catalog/pg_control.h" #include "common/controldata_utils.h" +#include "common/file_perm.h" #include "port/pg_crc32c.h" #ifndef FRONTEND #include "storage/fd.h" @@ -137,3 +139,94 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p) return ControlFile; } + +/* + * update_controlfile + * + * Update controlfile values with the contents given by caller. The + * contents to write are included in "ControlFile". Note that it is up + * to the caller to fsync the updated file. + */ +void +update_controlfile(const char *DataDir, const char *progname, + ControlFileData *ControlFile) +{ + int fd; + char buffer[PG_CONTROL_FILE_SIZE]; + char ControlFilePath[MAXPGPATH]; + + /* + * Apply the same static assertions as in backend's WriteControlFile(). + */ + StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_MAX_SAFE_SIZE, + "pg_control is too large for atomic disk writes"); + 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); + +#ifndef FRONTEND + if ((fd = OpenTransientFile(ControlFilePath, O_WRONLY | PG_BINARY)) == -1) + ereport(PANIC, + (errcode_for_file_access(), + errmsg("could not open file \"%s\": %m", + ControlFilePath))); +#else + if ((fd = open(ControlFilePath, O_WRONLY | PG_BINARY, + pg_file_create_mode)) == -1) + { + fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), + progname, ControlFilePath, strerror(errno)); + exit(EXIT_FAILURE); + } +#endif + + 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; + +#ifndef FRONTEND + ereport(PANIC, + (errcode_for_file_access(), + errmsg("could not write file \"%s\": %m", + ControlFilePath))); +#else + fprintf(stderr, _("%s: could not write \"%s\": %s\n"), + progname, ControlFilePath, strerror(errno)); + exit(EXIT_FAILURE); +#endif + } + +#ifndef FRONTEND + if (CloseTransientFile(fd)) + ereport(PANIC, + (errcode_for_file_access(), + errmsg("could not close file \"%s\": %m", + ControlFilePath))); +#else + if (close(fd) < 0) + { + fprintf(stderr, _("%s: could not close file \"%s\": %s\n"), + progname, ControlFilePath, strerror(errno)); + exit(EXIT_FAILURE); + } +#endif +} diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h index 0ffa2000fc..95317ebacf 100644 --- a/src/include/common/controldata_utils.h +++ b/src/include/common/controldata_utils.h @@ -12,6 +12,10 @@ #include "catalog/pg_control.h" -extern ControlFileData *get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p); +extern ControlFileData *get_controlfile(const char *DataDir, + const char *progname, + bool *crc_ok_p); +extern void update_controlfile(const char *DataDir, const char *progname, + ControlFileData *ControlFile); #endif /* COMMON_CONTROLDATA_UTILS_H */ -- 2.20.1
From 494b9967c3c6dc58c248a46ebc093f1061565e7e Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 11 Mar 2019 12:43:18 +0900 Subject: [PATCH 2/4] Rename pg_verify_checksums to pg_checksums The current name is too generic and focuses only on verifying checksums. More options to control checksums for an offline cluster are going to be added. Documentation as well as all past references to the tool are updated. Author: Michael Paquier Discussion: https://postgr.es/m/20181221201616.gd4...@nighthawk.caipicrew.dd-dns.de --- doc/src/sgml/ref/allfiles.sgml | 2 +- ...erify_checksums.sgml => pg_checksums.sgml} | 24 +++++++++---------- doc/src/sgml/reference.sgml | 2 +- src/backend/replication/basebackup.c | 2 +- src/bin/Makefile | 2 +- src/bin/initdb/t/001_initdb.pl | 8 +++---- src/bin/pg_checksums/.gitignore | 3 +++ .../Makefile | 20 ++++++++-------- src/bin/pg_checksums/nls.mk | 4 ++++ .../pg_checksums.c} | 17 +++++++------ src/bin/pg_checksums/t/001_basic.pl | 8 +++++++ .../t/002_actions.pl | 18 +++++++------- src/bin/pg_verify_checksums/.gitignore | 3 --- src/bin/pg_verify_checksums/nls.mk | 4 ---- src/bin/pg_verify_checksums/t/001_basic.pl | 8 ------- 15 files changed, 64 insertions(+), 61 deletions(-) rename doc/src/sgml/ref/{pg_verify_checksums.sgml => pg_checksums.sgml} (79%) create mode 100644 src/bin/pg_checksums/.gitignore rename src/bin/{pg_verify_checksums => pg_checksums}/Makefile (53%) create mode 100644 src/bin/pg_checksums/nls.mk rename src/bin/{pg_verify_checksums/pg_verify_checksums.c => pg_checksums/pg_checksums.c} (94%) create mode 100644 src/bin/pg_checksums/t/001_basic.pl rename src/bin/{pg_verify_checksums => pg_checksums}/t/002_actions.pl (89%) delete mode 100644 src/bin/pg_verify_checksums/.gitignore delete mode 100644 src/bin/pg_verify_checksums/nls.mk delete mode 100644 src/bin/pg_verify_checksums/t/001_basic.pl diff --git a/doc/src/sgml/ref/allfiles.sgml b/doc/src/sgml/ref/allfiles.sgml index c81c87ef41..f10d42ed84 100644 --- a/doc/src/sgml/ref/allfiles.sgml +++ b/doc/src/sgml/ref/allfiles.sgml @@ -199,6 +199,7 @@ Complete list of usable sgml source files in this directory. <!ENTITY pgarchivecleanup SYSTEM "pgarchivecleanup.sgml"> <!ENTITY pgBasebackup SYSTEM "pg_basebackup.sgml"> <!ENTITY pgbench SYSTEM "pgbench.sgml"> +<!ENTITY pgChecksums SYSTEM "pg_checksums.sgml"> <!ENTITY pgConfig SYSTEM "pg_config-ref.sgml"> <!ENTITY pgControldata SYSTEM "pg_controldata.sgml"> <!ENTITY pgCtl SYSTEM "pg_ctl-ref.sgml"> @@ -210,7 +211,6 @@ Complete list of usable sgml source files in this directory. <!ENTITY pgResetwal SYSTEM "pg_resetwal.sgml"> <!ENTITY pgRestore SYSTEM "pg_restore.sgml"> <!ENTITY pgRewind SYSTEM "pg_rewind.sgml"> -<!ENTITY pgVerifyChecksums SYSTEM "pg_verify_checksums.sgml"> <!ENTITY pgtestfsync SYSTEM "pgtestfsync.sgml"> <!ENTITY pgtesttiming SYSTEM "pgtesttiming.sgml"> <!ENTITY pgupgrade SYSTEM "pgupgrade.sgml"> diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml similarity index 79% rename from doc/src/sgml/ref/pg_verify_checksums.sgml rename to doc/src/sgml/ref/pg_checksums.sgml index 905b8f1222..6eec88afab 100644 --- a/doc/src/sgml/ref/pg_verify_checksums.sgml +++ b/doc/src/sgml/ref/pg_checksums.sgml @@ -1,27 +1,27 @@ <!-- -doc/src/sgml/ref/pg_verify_checksums.sgml +doc/src/sgml/ref/pg_checksums.sgml PostgreSQL documentation --> -<refentry id="pgverifychecksums"> - <indexterm zone="pgverifychecksums"> - <primary>pg_verify_checksums</primary> +<refentry id="pgchecksums"> + <indexterm zone="pgchecksums"> + <primary>pg_checksums</primary> </indexterm> <refmeta> - <refentrytitle><application>pg_verify_checksums</application></refentrytitle> + <refentrytitle><application>pg_checksums</application></refentrytitle> <manvolnum>1</manvolnum> <refmiscinfo>Application</refmiscinfo> </refmeta> <refnamediv> - <refname>pg_verify_checksums</refname> + <refname>pg_checksums</refname> <refpurpose>verify data checksums in a <productname>PostgreSQL</productname> database cluster</refpurpose> </refnamediv> <refsynopsisdiv> <cmdsynopsis> - <command>pg_verify_checksums</command> + <command>pg_checksums</command> <arg rep="repeat" choice="opt"><replaceable class="parameter">option</replaceable></arg> <group choice="opt"> <group choice="opt"> @@ -33,12 +33,12 @@ PostgreSQL documentation </cmdsynopsis> </refsynopsisdiv> - <refsect1 id="r1-app-pg_verify_checksums-1"> + <refsect1 id="r1-app-pg_checksums-1"> <title>Description</title> <para> - <command>pg_verify_checksums</command> verifies data checksums in a + <command>pg_checksums</command> verifies data checksums in a <productname>PostgreSQL</productname> cluster. The server must be shut - down cleanly before running <application>pg_verify_checksums</application>. + down cleanly before running <application>pg_checksums</application>. The exit status is zero if there are no checksum errors, otherwise nonzero. </para> </refsect1> @@ -84,7 +84,7 @@ PostgreSQL documentation <term><option>--version</option></term> <listitem> <para> - Print the <application>pg_verify_checksums</application> version and exit. + Print the <application>pg_checksums</application> version and exit. </para> </listitem> </varlistentry> @@ -94,7 +94,7 @@ PostgreSQL documentation <term><option>--help</option></term> <listitem> <para> - Show help about <application>pg_verify_checksums</application> command line + Show help about <application>pg_checksums</application> command line arguments, and exit. </para> </listitem> diff --git a/doc/src/sgml/reference.sgml b/doc/src/sgml/reference.sgml index db4f4167e3..cef09dd38b 100644 --- a/doc/src/sgml/reference.sgml +++ b/doc/src/sgml/reference.sgml @@ -276,6 +276,7 @@ &initdb; &pgarchivecleanup; + &pgChecksums; &pgControldata; &pgCtl; &pgResetwal; @@ -283,7 +284,6 @@ &pgtestfsync; &pgtesttiming; &pgupgrade; - &pgVerifyChecksums; &pgwaldump; &postgres; &postmaster; diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index 6c324a6661..537f09e342 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -190,7 +190,7 @@ static const char *excludeFiles[] = /* * List of files excluded from checksum validation. * - * Note: this list should be kept in sync with what pg_verify_checksums.c + * Note: this list should be kept in sync with what pg_checksums.c * includes. */ static const char *const noChecksumFiles[] = { diff --git a/src/bin/Makefile b/src/bin/Makefile index c66bfa887e..903e58121f 100644 --- a/src/bin/Makefile +++ b/src/bin/Makefile @@ -17,6 +17,7 @@ SUBDIRS = \ initdb \ pg_archivecleanup \ pg_basebackup \ + pg_checksums \ pg_config \ pg_controldata \ pg_ctl \ @@ -26,7 +27,6 @@ SUBDIRS = \ pg_test_fsync \ pg_test_timing \ pg_upgrade \ - pg_verify_checksums \ pg_waldump \ pgbench \ psql \ diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index 759779adb2..8dfcd8752a 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -63,12 +63,12 @@ mkdir $datadir; command_like(['pg_controldata', $datadir], qr/Data page checksum version:.*0/, 'checksums are disabled in control file'); -# pg_verify_checksums fails with checksums disabled by default. This is -# not part of the tests included in pg_verify_checksums to save from +# pg_checksums fails with checksums disabled by default. This is +# not part of the tests included in pg_checksums to save from # the creation of an extra instance. command_fails( - [ 'pg_verify_checksums', '-D', $datadir], - "pg_verify_checksums fails with data checksum disabled"); + [ 'pg_checksums', '-D', $datadir], + "pg_checksums fails with data checksum disabled"); command_ok([ 'initdb', '-S', $datadir ], 'sync only'); command_fails([ 'initdb', $datadir ], 'existing data directory'); diff --git a/src/bin/pg_checksums/.gitignore b/src/bin/pg_checksums/.gitignore new file mode 100644 index 0000000000..7888625094 --- /dev/null +++ b/src/bin/pg_checksums/.gitignore @@ -0,0 +1,3 @@ +/pg_checksums + +/tmp_check/ diff --git a/src/bin/pg_verify_checksums/Makefile b/src/bin/pg_checksums/Makefile similarity index 53% rename from src/bin/pg_verify_checksums/Makefile rename to src/bin/pg_checksums/Makefile index ab6d3ea9e2..278b7a0f2e 100644 --- a/src/bin/pg_verify_checksums/Makefile +++ b/src/bin/pg_checksums/Makefile @@ -1,38 +1,38 @@ #------------------------------------------------------------------------- # -# Makefile for src/bin/pg_verify_checksums +# Makefile for src/bin/pg_checksums # # Copyright (c) 1998-2019, PostgreSQL Global Development Group # -# src/bin/pg_verify_checksums/Makefile +# src/bin/pg_checksums/Makefile # #------------------------------------------------------------------------- -PGFILEDESC = "pg_verify_checksums - verify data checksums in an offline cluster" +PGFILEDESC = "pg_checksums - verify data checksums in an offline cluster" PGAPPICON=win32 -subdir = src/bin/pg_verify_checksums +subdir = src/bin/pg_checksums top_builddir = ../../.. include $(top_builddir)/src/Makefile.global -OBJS= pg_verify_checksums.o $(WIN32RES) +OBJS= pg_checksums.o $(WIN32RES) -all: pg_verify_checksums +all: pg_checksums -pg_verify_checksums: $(OBJS) | submake-libpgport +pg_checksums: $(OBJS) | submake-libpgport $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) install: all installdirs - $(INSTALL_PROGRAM) pg_verify_checksums$(X) '$(DESTDIR)$(bindir)/pg_verify_checksums$(X)' + $(INSTALL_PROGRAM) pg_checksums$(X) '$(DESTDIR)$(bindir)/pg_checksums$(X)' installdirs: $(MKDIR_P) '$(DESTDIR)$(bindir)' uninstall: - rm -f '$(DESTDIR)$(bindir)/pg_verify_checksums$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_checksums$(X)' clean distclean maintainer-clean: - rm -f pg_verify_checksums$(X) $(OBJS) + rm -f pg_checksums$(X) $(OBJS) rm -rf tmp_check check: diff --git a/src/bin/pg_checksums/nls.mk b/src/bin/pg_checksums/nls.mk new file mode 100644 index 0000000000..2748b18ef7 --- /dev/null +++ b/src/bin/pg_checksums/nls.mk @@ -0,0 +1,4 @@ +# src/bin/pg_checksums/nls.mk +CATALOG_NAME = pg_checksums +AVAIL_LANGUAGES = +GETTEXT_FILES = pg_checksums.c diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_checksums/pg_checksums.c similarity index 94% rename from src/bin/pg_verify_checksums/pg_verify_checksums.c rename to src/bin/pg_checksums/pg_checksums.c index 511262ab5f..f95e39f31e 100644 --- a/src/bin/pg_verify_checksums/pg_verify_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -1,12 +1,15 @@ -/* - * pg_verify_checksums +/*------------------------------------------------------------------------- + * pg_checksums.c * - * Verifies page level checksums in an offline cluster + * Verifies page level checksums in an offline cluster. * - * Copyright (c) 2010-2019, PostgreSQL Global Development Group + * Copyright (c) 2010-2019, PostgreSQL Global Development Group * - * src/bin/pg_verify_checksums/pg_verify_checksums.c + * IDENTIFICATION + * src/bin/pg_checksums/pg_checksums.c + *------------------------------------------------------------------------- */ + #include "postgres_fe.h" #include <dirent.h> @@ -240,7 +243,7 @@ main(int argc, char *argv[]) int option_index; bool crc_ok; - set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verify_checksums")); + set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_checksums")); progname = get_progname(argv[0]); @@ -253,7 +256,7 @@ main(int argc, char *argv[]) } if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0) { - puts("pg_verify_checksums (PostgreSQL) " PG_VERSION); + puts("pg_checksums (PostgreSQL) " PG_VERSION); exit(0); } } diff --git a/src/bin/pg_checksums/t/001_basic.pl b/src/bin/pg_checksums/t/001_basic.pl new file mode 100644 index 0000000000..4334c80606 --- /dev/null +++ b/src/bin/pg_checksums/t/001_basic.pl @@ -0,0 +1,8 @@ +use strict; +use warnings; +use TestLib; +use Test::More tests => 8; + +program_help_ok('pg_checksums'); +program_version_ok('pg_checksums'); +program_options_handling_ok('pg_checksums'); diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl similarity index 89% rename from src/bin/pg_verify_checksums/t/002_actions.pl rename to src/bin/pg_checksums/t/002_actions.pl index 74ad5ad723..97284e8930 100644 --- a/src/bin/pg_verify_checksums/t/002_actions.pl +++ b/src/bin/pg_checksums/t/002_actions.pl @@ -1,4 +1,4 @@ -# Do basic sanity checks supported by pg_verify_checksums using +# Do basic sanity checks supported by pg_checksums using # an initialized cluster. use strict; @@ -38,7 +38,7 @@ sub check_relation_corruption # Checksums are correct for single relfilenode as the table is not # corrupted yet. - command_ok(['pg_verify_checksums', '-D', $pgdata, + command_ok(['pg_checksums', '-D', $pgdata, '-r', $relfilenode_corrupted], "succeeds for single relfilenode on tablespace $tablespace with offline cluster"); @@ -49,7 +49,7 @@ sub check_relation_corruption close $file; # Checksum checks on single relfilenode fail - $node->command_checks_all([ 'pg_verify_checksums', '-D', $pgdata, '-r', + $node->command_checks_all([ 'pg_checksums', '-D', $pgdata, '-r', $relfilenode_corrupted], 1, [qr/Bad checksums:.*1/], @@ -57,7 +57,7 @@ sub check_relation_corruption "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_checksums', '-D', $pgdata], 1, [qr/Bad checksums:.*1/], [qr/checksum verification failed/], @@ -67,7 +67,7 @@ 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_checksums', '-D', $pgdata], "succeeds again after table drop on tablespace $tablespace"); $node->start; @@ -101,12 +101,12 @@ mkdir "$pgdata/global/pgsql_tmp"; append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo"; # Checksums pass on a newly-created cluster -command_ok(['pg_verify_checksums', '-D', $pgdata], +command_ok(['pg_checksums', '-D', $pgdata], "succeeds with offline cluster"); # Checks cannot happen with an online cluster $node->start; -command_fails(['pg_verify_checksums', '-D', $pgdata], +command_fails(['pg_checksums', '-D', $pgdata], "fails with online cluster"); # Check corruption of table on default tablespace. @@ -121,7 +121,7 @@ $node->safe_psql('postgres', "CREATE TABLESPACE ts_corrupt LOCATION '$tablespace_dir';"); check_relation_corruption($node, 'corrupt2', 'ts_corrupt'); -# Utility routine to check that pg_verify_checksums is able to detect +# Utility routine to check that pg_checksums is able to detect # correctly-named relation files filled with some corrupted data. sub fail_corrupt { @@ -133,7 +133,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_checksums', '-D', $pgdata], 1, [qr/^$/], [qr/could not read block 0 in file.*$file\":/], diff --git a/src/bin/pg_verify_checksums/.gitignore b/src/bin/pg_verify_checksums/.gitignore deleted file mode 100644 index 0e5e569a54..0000000000 --- a/src/bin/pg_verify_checksums/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -/pg_verify_checksums - -/tmp_check/ diff --git a/src/bin/pg_verify_checksums/nls.mk b/src/bin/pg_verify_checksums/nls.mk deleted file mode 100644 index 893efaf0f0..0000000000 --- a/src/bin/pg_verify_checksums/nls.mk +++ /dev/null @@ -1,4 +0,0 @@ -# src/bin/pg_verify_checksums/nls.mk -CATALOG_NAME = pg_verify_checksums -AVAIL_LANGUAGES = -GETTEXT_FILES = pg_verify_checksums.c diff --git a/src/bin/pg_verify_checksums/t/001_basic.pl b/src/bin/pg_verify_checksums/t/001_basic.pl deleted file mode 100644 index 1fa2e12db2..0000000000 --- a/src/bin/pg_verify_checksums/t/001_basic.pl +++ /dev/null @@ -1,8 +0,0 @@ -use strict; -use warnings; -use TestLib; -use Test::More tests => 8; - -program_help_ok('pg_verify_checksums'); -program_version_ok('pg_verify_checksums'); -program_options_handling_ok('pg_verify_checksums'); -- 2.20.1
From d86b8a1fa87f0244d8eaaffe21de2290e7732d76 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 11 Mar 2019 13:45:44 +0900 Subject: [PATCH 3/4] Add options to enable and disable checksums in pg_checksums An offline cluster can now work with more modes in pg_checksums: - --enable can enable checksums in a cluster, updating all blocks with a correct checksum, and update the control file at the end. - --disable can disable checksums in a cluster, updating the the control file. - --check is an extra option able to verify checksums for a cluster. When running --enable or --disable, the data folder gets fsync'd for durability. If no mode is specified in the options, then --check is used for compatibility with older versions of pg_verify_checksums (now renamed to pg_checksums in v12). Author: Michael Banck Reviewed-by: Fabien Coelho, Michael Paquier Discussion: https://postgr.es/m/20181221201616.gd4...@nighthawk.caipicrew.dd-dns.de --- doc/src/sgml/ref/pg_checksums.sgml | 50 +++++++- src/bin/pg_checksums/pg_checksums.c | 171 ++++++++++++++++++++++---- src/bin/pg_checksums/t/002_actions.pl | 76 +++++++++--- 3 files changed, 251 insertions(+), 46 deletions(-) diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml index 6eec88afab..89f2f2b0e9 100644 --- a/doc/src/sgml/ref/pg_checksums.sgml +++ b/doc/src/sgml/ref/pg_checksums.sgml @@ -16,7 +16,7 @@ PostgreSQL documentation <refnamediv> <refname>pg_checksums</refname> - <refpurpose>verify data checksums in a <productname>PostgreSQL</productname> database cluster</refpurpose> + <refpurpose>enable, disable or check data checksums in a <productname>PostgreSQL</productname> database cluster</refpurpose> </refnamediv> <refsynopsisdiv> @@ -36,10 +36,19 @@ PostgreSQL documentation <refsect1 id="r1-app-pg_checksums-1"> <title>Description</title> <para> - <command>pg_checksums</command> verifies data checksums in a - <productname>PostgreSQL</productname> cluster. The server must be shut - down cleanly before running <application>pg_checksums</application>. - The exit status is zero if there are no checksum errors, otherwise nonzero. + <command>pg_checksums</command> checks, enables or disables data + checksums in a <productname>PostgreSQL</productname> cluster. The server + must be shut down cleanly before running + <application>pg_checksums</application>. The exit status is zero if there + are no checksum errors when checking them, and nonzero if at least one + checksum failure is detected. If enabling or disabling checksums, the + exit status is nonzero if the operation failed. + </para> + + <para> + While checking or enabling checksums needs to scan or write every file in + the cluster, disabling will only update the file + <filename>pg_control</filename>. </para> </refsect1> @@ -60,6 +69,37 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>-c</option></term> + <term><option>--check</option></term> + <listitem> + <para> + Checks checksums. This is the default mode if nothing else is + specified. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><option>-d</option></term> + <term><option>--disable</option></term> + <listitem> + <para> + Disables checksums. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><option>-e</option></term> + <term><option>--enable</option></term> + <listitem> + <para> + Enables checksums. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>-v</option></term> <term><option>--verbose</option></term> diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index f95e39f31e..366f397e3f 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -1,7 +1,7 @@ /*------------------------------------------------------------------------- * pg_checksums.c * - * Verifies page level checksums in an offline cluster. + * Checks, enables or disables page level checksums in an offline cluster * * Copyright (c) 2010-2019, PostgreSQL Global Development Group * @@ -16,14 +16,15 @@ #include <sys/stat.h> #include <unistd.h> -#include "catalog/pg_control.h" +#include "access/xlog_internal.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; @@ -34,16 +35,40 @@ static ControlFileData *ControlFile; static char *only_relfilenode = NULL; static bool verbose = false; +typedef enum +{ + PG_ACTION_CHECK, + PG_ACTION_DISABLE, + PG_ACTION_ENABLE +} ChecksumAction; + +/* + * Filename components. + * + * XXX: fd.h is not declared here as frontend side code is not able to + * interact with the backend-side definitions for the various fsync + * wrappers. + */ +#define PG_TEMP_FILES_DIR "pgsql_tmp" +#define PG_TEMP_FILE_PREFIX "pgsql_tmp" + +static ChecksumAction action = PG_ACTION_CHECK; + 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 or verifies data checksums in a PostgreSQL\n"), progname); + printf(_("database cluster.\n\n")); printf(_("Usage:\n")); printf(_(" %s [OPTION]... [DATADIR]\n"), progname); printf(_("\nOptions:\n")); printf(_(" [-D, --pgdata=]DATADIR data directory\n")); + printf(_(" -c, --check check data checksums. This is the default\n")); + printf(_(" mode if nothing is specified.\n")); + printf(_(" -d, --disable disable data checksums\n")); + printf(_(" -e, --enable enable data checksums\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")); @@ -89,8 +114,14 @@ scan_file(const char *fn, BlockNumber segmentno) PageHeader header = (PageHeader) buf.data; int f; BlockNumber blockno; + int flags; + + Assert(action == PG_ACTION_ENABLE || + action == PG_ACTION_CHECK); + + flags = (action == PG_ACTION_ENABLE) ? O_RDWR : O_RDONLY; + f = open(fn, PG_BINARY | flags, 0); - f = open(fn, O_RDONLY | PG_BINARY, 0); if (f < 0) { fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), @@ -120,18 +151,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_CHECK) { - 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_CHECK) + 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); } @@ -233,7 +293,10 @@ int main(int argc, char *argv[]) { static struct option long_options[] = { + {"check", 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} }; @@ -261,10 +324,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_CHECK; + break; + case 'd': + action = PG_ACTION_DISABLE; + break; + case 'e': + action = PG_ACTION_ENABLE; + break; case 'v': verbose = true; break; @@ -311,6 +383,15 @@ main(int argc, char *argv[]) exit(1); } + /* Relfilenode checking only works in --check mode */ + if (action != PG_ACTION_CHECK && only_relfilenode) + { + fprintf(stderr, _("%s: relfilenode option only possible with --check\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) @@ -322,29 +403,67 @@ 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_CHECK) { 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 > 0 && + action == PG_ACTION_ENABLE) + { + fprintf(stderr, _("%s: data checksums are already enabled in cluster.\n"), progname); + exit(1); + } - /* Scan all files */ - scan_directory(DataDir, "global"); - scan_directory(DataDir, "base"); - scan_directory(DataDir, "pg_tblspc"); + /* Operate on all files if checking or enabling checksums */ + if (action == PG_ACTION_CHECK || action == PG_ACTION_ENABLE) + { + 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(_("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)); + printf(_("Checksum operation completed\n")); + printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files)); + printf(_("Blocks scanned: %s\n"), psprintf(INT64_FORMAT, blocks)); + if (action == PG_ACTION_CHECK) + { + printf(_("Bad checksums: %s\n"), psprintf(INT64_FORMAT, badblocks)); + printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version); - if (badblocks > 0) - return 1; + if (badblocks > 0) + return 1; + } + } + + /* + * Finally update the control file, flushing the data directory at the + * end. + */ + if (action == PG_ACTION_ENABLE || action == PG_ACTION_DISABLE) + { + /* Update control file */ + ControlFile->data_checksum_version = + (action == PG_ACTION_ENABLE) ? PG_DATA_CHECKSUM_VERSION : 0; + update_controlfile(DataDir, progname, ControlFile); + fsync_pgdata(DataDir, progname, PG_VERSION_NUM); + if (verbose) + printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version); + if (action == PG_ACTION_ENABLE) + printf(_("Checksums enabled in cluster\n")); + else + printf(_("Checksums disabled in cluster\n")); + } return 0; } diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl index 97284e8930..58be8d5cb6 100644 --- a/src/bin/pg_checksums/t/002_actions.pl +++ b/src/bin/pg_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 => 62; # 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_checksums', '-D', $pgdata, - '-r', $relfilenode_corrupted], + command_ok(['pg_checksums', '--check', '-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_checksums', '-D', $pgdata, '-r', - $relfilenode_corrupted], + $node->command_checks_all([ 'pg_checksums', '--check', '-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_checksums', '-D', $pgdata], + $node->command_checks_all([ 'pg_checksums', '--check', '-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_checksums', '-D', $pgdata], + $node->command_ok(['pg_checksums', '--check', '-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,59 @@ 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_checksums', '--enable', '-D', $pgdata], + "checksums successfully enabled in cluster"); + +# Successive attempt to enable checksums fails. +command_fails(['pg_checksums', '--enable', '-D', $pgdata], + "enabling checksums fails if already enabled"); + +# 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_checksums', '--disable', '-D', $pgdata], + "checksums successfully disabled in cluster"); + +# Successive attempt to disable checksums fails. +command_fails(['pg_checksums', '--disable', '-D', $pgdata], + "disabling checksums fails if already disabled"); + +# 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 for follow-up tests. +command_ok(['pg_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_checksums', '-D', $pgdata], +command_ok(['pg_checksums', '--check', '-D', $pgdata], "succeeds with offline cluster"); +# Checksums are verified if no other arguments are specified +command_ok(['pg_checksums', '-D', $pgdata], + "verifies checksums as default action"); + +# Specific relation files cannot be requested when action is --disable +# or --enable. +command_fails(['pg_checksums', '--disable', '-r', '1234', '-D', $pgdata], + "fails when relfilnodes are requested and action is --disable"); +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata], + "fails when relfilnodes are requested and action is --disable"); + # Checks cannot happen with an online cluster $node->start; -command_fails(['pg_checksums', '-D', $pgdata], +command_fails(['pg_checksums', '--check', '-D', $pgdata], "fails with online cluster"); # Check corruption of table on default tablespace. @@ -133,7 +179,7 @@ sub fail_corrupt my $file_name = "$pgdata/global/$file"; append_to_file $file_name, "foo"; - $node->command_checks_all([ 'pg_checksums', '-D', $pgdata], + $node->command_checks_all([ 'pg_checksums', '--check', '-D', $pgdata], 1, [qr/^$/], [qr/could not read block 0 in file.*$file\":/], -- 2.20.1
From bd3a64fea68d67eb349e12cf222fd86919419bd1 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 11 Mar 2019 13:46:09 +0900 Subject: [PATCH 4/4] Add option -N/--no-sync to pg_checksums This is an option consistent with what pg_dump, pg_rewind and pg_basebackup provide which is useful for leveraging the I/O effort when testing things, not to be used in a production environment. Author: Michael Paquier Discussion: https://postgr.es/m/20181221201616.gd4...@nighthawk.caipicrew.dd-dns.de --- doc/src/sgml/ref/pg_checksums.sgml | 16 ++++++++++++++++ src/bin/pg_checksums/pg_checksums.c | 11 +++++++++-- src/bin/pg_checksums/t/002_actions.pl | 10 +++++----- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml index 89f2f2b0e9..2819335224 100644 --- a/doc/src/sgml/ref/pg_checksums.sgml +++ b/doc/src/sgml/ref/pg_checksums.sgml @@ -100,6 +100,22 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>-N</option></term> + <term><option>--no-sync</option></term> + <listitem> + <para> + By default, <command>pg_checksums</command> will wait for all files + to be written safely to disk. This option causes + <command>pg_checksums</command> to return without waiting, which is + faster, but means that a subsequent operating system crash can leave + the updated data folder corrupt. Generally, this option is useful + for testing but should not be used on a production installation. + This option has no effect when using <literal>--check</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>-v</option></term> <term><option>--verbose</option></term> diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 366f397e3f..36f66ba872 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -33,6 +33,7 @@ static int64 badblocks = 0; static ControlFileData *ControlFile; static char *only_relfilenode = NULL; +static bool do_sync = true; static bool verbose = false; typedef enum @@ -69,6 +70,7 @@ usage(void) printf(_(" mode if nothing is specified.\n")); printf(_(" -d, --disable disable data checksums\n")); printf(_(" -e, --enable enable data checksums\n")); + printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\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")); @@ -297,6 +299,7 @@ main(int argc, char *argv[]) {"pgdata", required_argument, NULL, 'D'}, {"disable", no_argument, NULL, 'd'}, {"enable", no_argument, NULL, 'e'}, + {"no-sync", no_argument, NULL, 'N'}, {"verbose", no_argument, NULL, 'v'}, {NULL, 0, NULL, 0} }; @@ -324,7 +327,7 @@ main(int argc, char *argv[]) } } - while ((c = getopt_long(argc, argv, "cD:der:v", long_options, &option_index)) != -1) + while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1) { switch (c) { @@ -337,6 +340,9 @@ main(int argc, char *argv[]) case 'e': action = PG_ACTION_ENABLE; break; + case 'N': + do_sync = false; + break; case 'v': verbose = true; break; @@ -456,7 +462,8 @@ main(int argc, char *argv[]) ControlFile->data_checksum_version = (action == PG_ACTION_ENABLE) ? PG_DATA_CHECKSUM_VERSION : 0; update_controlfile(DataDir, progname, ControlFile); - fsync_pgdata(DataDir, progname, PG_VERSION_NUM); + if (do_sync) + fsync_pgdata(DataDir, progname, PG_VERSION_NUM); if (verbose) printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version); if (action == PG_ACTION_ENABLE) diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl index 58be8d5cb6..ff9bb70040 100644 --- a/src/bin/pg_checksums/t/002_actions.pl +++ b/src/bin/pg_checksums/t/002_actions.pl @@ -101,11 +101,11 @@ mkdir "$pgdata/global/pgsql_tmp"; append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo"; # Enable checksums. -command_ok(['pg_checksums', '--enable', '-D', $pgdata], +command_ok(['pg_checksums', '--enable', '--no-sync', '-D', $pgdata], "checksums successfully enabled in cluster"); # Successive attempt to enable checksums fails. -command_fails(['pg_checksums', '--enable', '-D', $pgdata], +command_fails(['pg_checksums', '--enable', '--no-sync', '-D', $pgdata], "enabling checksums fails if already enabled"); # Control file should know that checksums are enabled. @@ -113,12 +113,12 @@ command_like(['pg_controldata', $pgdata], qr/Data page checksum version:.*1/, 'checksums enabled in control file'); -# Disable checksums again. +# Disable checksums again. Flush result here as that should be cheap. command_ok(['pg_checksums', '--disable', '-D', $pgdata], "checksums successfully disabled in cluster"); # Successive attempt to disable checksums fails. -command_fails(['pg_checksums', '--disable', '-D', $pgdata], +command_fails(['pg_checksums', '--disable', '--no-sync', '-D', $pgdata], "disabling checksums fails if already disabled"); # Control file should know that checksums are disabled. @@ -127,7 +127,7 @@ command_like(['pg_controldata', $pgdata], 'checksums disabled in control file'); # Enable checksums again for follow-up tests. -command_ok(['pg_checksums', '--enable', '-D', $pgdata], +command_ok(['pg_checksums', '--enable', '--no-sync', '-D', $pgdata], "checksums successfully enabled in cluster"); # Control file should know that checksums are enabled. -- 2.20.1
signature.asc
Description: PGP signature
