On Mon, Mar 11, 2019 at 02:11:11PM +0000, Sergei Kornilov wrote: > I review latest patchset.
Thanks, I have committed the refactoring of src/common/ as a first
step.
> I have one big question: Is pg_checksums
> safe for cross-versions operations? Even with update_controlfile
> call? Currently i am able to enable checksums in pg11 cluster with
> pg_checksums compiled on HEAD. Is this expected? I didn't notice any
> version-specific check in code.
This depends on the version of the control file, and it happens that
we don't check for it, so that's a good catch from your side. Not
doing the check is a bad idea as ControlFileData should be compatible
between the binary and the data read. I am attaching a fresh 0001
which should be back-patched down to v11 as a bug fix. An advantage
of that, which is similar to pg_rewind, is that if the control file
version does not change in a major version, then the tool can be
used. And the data folder layer is unlikely going to change..
>> <command>pg_checksums</command> checks, enables or disables data checksums
>
> maybe better is <application>, not <command>?
Fixed, as part of the renaming patch.
>> + printf(_("%s enables, disables or verifies data checksums in a
>> PostgreSQL\n"), progname);
>> + printf(_("database cluster.\n\n"));
>
> I doubt this is good line formatting for translation purposes.
>
>> + printf(_(" -c, --check check data checksums. This is the
>> default\n"));
>> + printf(_(" mode if nothing is specified.\n"));
>
> same. For example pg_basebackup uses different multiline style:
Oh, good points. I forgot about that point of view.
>> +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata],
>> + "fails when relfilnodes are requested and action is --disable");
>
> action is "--enable" here ;-)
s/relfilnodes/relfilenodes/ while on it.
>> if (badblocks > 0)gi
>> return 1;
>
> Small question: why return 1 instead of exit(1)?
OK, let's fix that on the way as part of the renaming.
>> <refentry id="pgchecksums">
>> <indexterm zone="pgchecksums">
>
> How about use "app-pgchecksums" similar to other applications?
Yes, I was wondering about that one when doing the renaming, but did
not bother much for consistency. Anyway switched, you are right.
Attached is an updated patch set, minus the refactoring for
src/common/.
--
Michael
From ef3e4f9b0dbff7deaaf19cb303f71c7883193a72 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 12 Mar 2019 10:43:47 +0900 Subject: [PATCH v2 1/4] Ensure version compatibility of pg_verify_checksums pg_verify_checksums performs a read of the control file, and the data it fetches should be from a data folder compatible with the major version of Postgres the binary has been compiled with. Reported-by: Sergei Kornilov Author: Michael Paquier Discussion: https://postgr.es/m/155231347133.16480.11453587097036807558.p...@coridan.postgresql.org Backpatch-through: 11, where the tool has been introduced. --- src/bin/pg_verify_checksums/pg_verify_checksums.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c index 511262ab5f..4c7c055b31 100644 --- a/src/bin/pg_verify_checksums/pg_verify_checksums.c +++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c @@ -316,6 +316,13 @@ main(int argc, char *argv[]) exit(1); } + if (ControlFile->pg_control_version != PG_CONTROL_VERSION) + { + fprintf(stderr, _("%s: cluster is not compatible with this version of pg_verify_checksums\n"), + progname); + exit(1); + } + if (ControlFile->state != DB_SHUTDOWNED && ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY) { -- 2.20.1
From deee88e2d95f69b360580fbdd5300d2c34cda58b Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 12 Mar 2019 11:02:05 +0900 Subject: [PATCH v2 2/4] Rename pg_verify_checksums to pg_checksums The current tool name is too restrictive and focuses only on verifying checksums. As more options to control checksums for an offline cluster are planned to be added, switch to a more generic name. Documentation as well as all past references to the tool are updated. Author: Michael Paquier Reviewed-by: Michael Banck, Seigei Kornilov 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} | 20 +++++++++------- 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, 66 insertions(+), 62 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..8d91f3529e 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..6a47dda683 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="app-pgchecksums"> + <indexterm zone="app-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 + <application>pg_checksums</application> 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 4c7c055b31..6571c34211 100644 --- a/src/bin/pg_verify_checksums/pg_verify_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -1,12 +1,16 @@ -/* - * pg_verify_checksums +/*------------------------------------------------------------------------- * - * Verifies page level checksums in an offline cluster + * pg_checksums.c + * 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 +244,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 +257,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); } } @@ -318,7 +322,7 @@ main(int argc, char *argv[]) if (ControlFile->pg_control_version != PG_CONTROL_VERSION) { - fprintf(stderr, _("%s: cluster is not compatible with this version of pg_verify_checksums\n"), + fprintf(stderr, _("%s: cluster is not compatible with this version of pg_checksums\n"), progname); exit(1); } 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 f07d248aa82d4838d23c45238ce2147d31b8727d Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 12 Mar 2019 11:12:03 +0900 Subject: [PATCH v2 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 6a47dda683..776f7be477 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> - <application>pg_checksums</application> 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. + <application>pg_checksums</application> 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 6571c34211..7d9c44c361 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -1,7 +1,8 @@ /*------------------------------------------------------------------------- * * pg_checksums.c - * Verifies page level checksums in an offline cluster. + * Checks, enables or disables page level checksums for an offline + * cluster * * Copyright (c) 2010-2019, PostgreSQL Global Development Group * @@ -17,14 +18,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; @@ -35,16 +37,39 @@ 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 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(_(" -c, --check check data checksums\n")); + printf(_(" This is the default 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")); @@ -90,8 +115,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"), @@ -121,18 +152,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); } @@ -234,7 +294,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} }; @@ -262,10 +325,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; @@ -312,6 +384,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) @@ -330,29 +411,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..3ab18a6b89 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 relfilenodes are requested and action is --disable"); +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata], + "fails when relfilenodes are requested and action is --enable"); + # 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 9ac23f54948f697d7b8591e4880dab354718790e Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 11 Mar 2019 13:46:09 +0900 Subject: [PATCH v2 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 776f7be477..c3ccbf4eb7 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 7d9c44c361..8edbeefc91 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -35,6 +35,7 @@ static int64 badblocks = 0; static ControlFileData *ControlFile; static char *only_relfilenode = NULL; +static bool do_sync = true; static bool verbose = false; typedef enum @@ -70,6 +71,7 @@ usage(void) printf(_(" This is the default 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")); @@ -298,6 +300,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} }; @@ -325,7 +328,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) { @@ -338,6 +341,9 @@ main(int argc, char *argv[]) case 'e': action = PG_ACTION_ENABLE; break; + case 'N': + do_sync = false; + break; case 'v': verbose = true; break; @@ -464,7 +470,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 3ab18a6b89..41575c5245 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
