From: | Michael Banck <michael(dot)banck(at)credativ(dot)de> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Magnus Hagander <magnus(at)hagander(dot)net>, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, Sergei Kornilov <sk(at)zsrv(dot)org>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Offline enabling/disabling of data checksums |
Date: | 2019-03-15 12:37:27 |
Message-ID: | 1552653447.9697.24.camel@credativ.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)banck(at)credativ(dot)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 | Date | Subject | |
---|---|---|---|
Next Message | Chris Travers | 2019-03-15 12:57:40 | Re: Re: Re: [HACKERS] Custom compression methods |
Previous Message | Michael Paquier | 2019-03-15 12:23:09 | Re: Offline enabling/disabling of data checksums |