Message-ID: <1552637091.9697.18.camel@credativ.de>
Subject: Re: Offline enabling/disabling of data checksums
From: Michael Banck <michael.banck@credativ.de>
To: Michael Paquier <michael@paquier.xyz>
Cc: Magnus Hagander <magnus@hagander.net>, Fabien COELHO
 <coelho@cri.ensmp.fr>,  Sergei Kornilov <sk@zsrv.org>, PostgreSQL
 Developers <pgsql-hackers@lists.postgresql.org>
Date: Fri, 15 Mar 2019 09:04:51 +0100
In-Reply-To: <20190315025027.GN3493@paquier.xyz>
References: <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>
	 <1552475364.4947.64.camel@credativ.de> <20190313115633.GA1878@paquier.xyz>
	 <20190314043936.GH3493@paquier.xyz>
	 <CABUevExjCOKDRX=B6fBkoYs7Q2jYJLZS4Bxh1pi0b2_BV-7DEA@mail.gmail.com>
	 <1552577180.9697.4.camel@credativ.de> <20190315025027.GN3493@paquier.xyz>
Organization: credativ GmbH
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

Am Freitag, den 15.03.2019, 11:50 +0900 schrieb Michael Paquier:
> On Thu, Mar 14, 2019 at 04:26:20PM +0100, Michael Banck wrote:
> > Am Donnerstag, den 14.03.2019, 15:26 +0100 schrieb Magnus Hagander:
> > > One big-hammer method could be similar to what pg_upgrade does --
> > > temporarily rename away the controlfile so postgresql can't start, and
> > > when done, put it back.
> > 
> > That sounds like a good solution to me. I've made PoC patch for that,
> > see attached.
> 
> Indeed.  I did not know this trick from pg_upgrade.  We could just use
> the same.
> 
> > The only question is whether pg_checksums should try to move pg_control
> > back (i) on failure (ii) when interrupted?
> 
> Yes, we should have a callback on SIGINT and SIGTERM here which just
> moves back in place the control file if the temporary one exists.  I
> have been able to grab some time to incorporate the feedback gathered
> on this thread, and please find attached a new version of the patch to
> add --enable/--disable.  

Thanks!

One thing stood out to me while quickly looking over it:

+		/*
+		 * Flush the control file and its parent path to make the change
+		 * durable.
+		 */
+		if (fsync_fname(controlfile_path, false, progname) != 0 ||
+			fsync_parent_path(controlfile_path, progname) != 0)
+		{
+			/* errors are already logged on failure */
+			exit(1);
+		}

ISTM this would not run fsync_parent_path() unless the first fsync fails
which is not the intended use. I guess we need two ifs here?


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/datenschutz