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.ba...@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.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
From 731c48d21b0adfa74e7ce1e05b2d8f1146b9e3cf Mon Sep 17 00:00:00 2001
From: Michael Banck <mba...@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.c
index 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
