Message-ID: <1552653447.9697.24.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 13:37:27 +0100
In-Reply-To: <20190315122309.GA1434@paquier.xyz>
References: <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>
	 <1552650841.9697.22.camel@credativ.de> <20190315122309.GA1434@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, 21:23 +0900 schrieb Michael Paquier:
> On Fri, Mar 15, 2019 at 12:54:01PM +0100, Michael Banck wrote:
> > 1. There's a typo in line 578 which makes it fail to compile:
> > 
> > > src/bin/pg_checksums/pg_checksums.c:578:4: error: ‘y’ undeclared (first use in this function)
> > >   }y
> 
> I am wondering where you got this one.  My local branch does not have
> it, and the patch I sent does not seem to have it either.

Mea culpa, I must've fat fingered something in the editor before
applying your patch, sorry. I should've double-checked.

> > 2. Should the pqsignal() stuff only be setup in PG_MODE_ENABLE? Same
> > with the controlfile_path?
> 
> PG_MODE_DISABLE needs controlfile_path as well.  We could make the
> cleanup only available when using --enable, the code just looked more
> simple in its current shape.  I think it's just more simple to set
> everything unconditionally.  This code may become more complicated in
> the future.

Ok.

> > 3. There's (I think) leftover debug output in the following places:
> > 
> > > +		printf(_("Renaming \"%s\" to \"%s\"\n"), controlfile_path,
> > > +				controlfile_path_temp);
> > > +		printf(_("Renaming \"%s\" to \"%s\"\n"), controlfile_path_temp,
> > > +				controlfile_path);
> > > +		printf(_("Syncing data folder\n"));
> > 
> > (that one is debatable, we are mentioning this only in verbose mode in
> > pg_basebackup but pg_checksums is more chatty anyway, so probably
> > fine).
> 
> This is wanted.  Many folks have been complaning on this thread about
> crashes and such, surely we want logs about what happens :)
> 
> > > +		printf(_("Updating control file\n"));
> > 
> > Besides to the syncing message (which is user-relevant cause they might
> > wonder what is taking so long), the others seem to be implementation
> > details we don't need to tell the user about.
> 
> Perhaps having them under --verbose makes more sense?

Well if we think it is essential in order to tell the user what happened
in the case of an error, it shouldn't be verbose I guess.


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