From: Michael Banck <michael.banck@credativ.de>
To: Magnus Hagander <magnus@hagander.net>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Offline enabling/disabling of data checksums
Date: Wed, 13 Mar 2019 12:09:24 +0100
Message-ID: <1552475364.4947.64.camel@credativ.de> (raw)
In-Reply-To: <CABUevExWFGMZktS2UEYfKbgsEKLVnrQxv5M8GMYFgiBmQy+cNA@mail.gmail.com>
References: <20181221201616.GD4974@nighthawk.caipicrew.dd-dns.de>
<20190311045323.GE1818@paquier.xyz>
<155231347133.16480.11453587097036807558.pgcf@coridan.postgresql.org>
<1552385626.4947.50.camel@credativ.de>
<20190312124403.GM13812@paquier.xyz>
<20190313021621.GP13812@paquier.xyz>
<alpine.DEB.2.21.1903130805330.4059@lancre>
<20190313093150.GE2988@paquier.xyz>
<1552473675.4947.62.camel@credativ.de>
<CABUevExWFGMZktS2UEYfKbgsEKLVnrQxv5M8GMYFgiBmQy+cNA@mail.gmail.com>
Hi,
Am Mittwoch, den 13.03.2019, 11:47 +0100 schrieb Magnus Hagander:
> On Wed, Mar 13, 2019 at 11:41 AM Michael Banck <michael.banck@credativ.de> wrote:
> > I propose we re-read the control file for the enable case after we
> > finished operating on all files and (i) check the instance is still
> > offline and (ii) update the checksums version from there. That should be
> > a small but worthwhile change that could be done anyway.
>
> In (i) you need to also check that is' not offline *again*. Somebody
> could start *and* stop the database while pg_checksums is running. But
> that should hopefully be enough to check the time field?
Good point.
> > Also, I suggest to maybe add a notice in verbose mode that we are
> > syncing the data directory - otherwise the user might wonder what's
> > going on at 100% done, though I haven't seen a large delay in my tests
> > so far.
>
> Seems like a good idea -- there certainly could be a substantial delay
> there depending on data size and underlying storage.
The attached patch should do the above, on top of Michael's last
patchset.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutzAttachments:
[text/x-patch] 0001-Guard-against-concurrent-cluster-changes-when-enabli.patch (3.2K, ../1552475364.4947.64.camel@credativ.de/2-0001-Guard-against-concurrent-cluster-changes-when-enabli.patch)
download | inline diff:
From 731c48d21b0adfa74e7ce1e05b2d8f1146b9e3cf Mon Sep 17 00:00:00 2001
From: Michael Banck <mbanck@debian.org>
Date: Wed, 13 Mar 2019 12:05:34 +0100
Subject: [PATCH] Guard against concurrent cluster changes when enabling
checksums.
Re-read the control file after operating on all files in order to check whether
the instance is still shutdown and the control file still has the same
modification timestamp.
In passing, add a note about syncing the data directory in verbose mode.
---
src/bin/pg_checksums/pg_checksums.c | 38 ++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.cindex 0e464299f3..afca6cf027 100644--- a/src/bin/pg_checksums/pg_checksums.c+++ b/src/bin/pg_checksums/pg_checksums.c@@ -309,6 +309,7 @@ main(int argc, char *argv[])
int c;
int option_index;
bool crc_ok;
+ pg_time_t controlfile_last_updated;
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_checksums"));
@@ -399,7 +400,7 @@ main(int argc, char *argv[])
exit(1);
}
- /* Check if cluster is running */+ /* Get control file data */
ControlFile = get_controlfile(DataDir, progname, &crc_ok);
if (!crc_ok)
{
@@ -414,6 +415,7 @@ main(int argc, char *argv[])
exit(1);
}
+ /* Check if cluster is running */
if (ControlFile->state != DB_SHUTDOWNED &&
ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
{
@@ -440,6 +442,9 @@ main(int argc, char *argv[])
exit(1);
}
+ /* Save time of last control file modification */+ controlfile_last_updated = ControlFile->time;+
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
@@ -461,17 +466,44 @@ main(int argc, char *argv[])
}
/*
- * Finally update the control file, flushing the data directory at the- * end.+ * Finally update the control file after checking the cluster is still+ * offline and its control file has not changed, flushing the data+ * directory at the end.
*/
if (mode == PG_MODE_ENABLE || mode == PG_MODE_DISABLE)
{
+ ControlFile = get_controlfile(DataDir, progname, &crc_ok);+ if (!crc_ok)+ {+ fprintf(stderr, _("%s: pg_control CRC value is incorrect\n"), progname);+ exit(1);+ }++ /* Check if cluster is running */+ if (ControlFile->state != DB_SHUTDOWNED &&+ ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)+ {+ fprintf(stderr, _("%s: cluster no longer shut down\n"), progname);+ exit(1);+ }++ /* Check if control file has changed */+ if (controlfile_last_updated != ControlFile->time)+ {+ fprintf(stderr, _("%s: control file has changed since startup\n"), progname);+ exit(1);+ }+
/* Update control file */
ControlFile->data_checksum_version =
(mode == PG_MODE_ENABLE) ? PG_DATA_CHECKSUM_VERSION : 0;
update_controlfile(DataDir, progname, ControlFile);
if (do_sync)
+ {+ if (verbose && mode == PG_MODE_ENABLE)+ printf(_("Syncing data directory\n"));
fsync_pgdata(DataDir, progname, PG_VERSION_NUM);
+ }
if (verbose)
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
if (mode == PG_MODE_ENABLE)
--
2.11.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael.banck@credativ.de, magnus@hagander.net, michael@paquier.xyz, coelho@cri.ensmp.fr, sk@zsrv.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Offline enabling/disabling of data checksums
In-Reply-To: <1552475364.4947.64.camel@credativ.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox