Re: Offline enabling/disabling of data checksums

On Mon, Mar 11, 2019 at 02:11:11PM +0000, Sergei Kornilov wrote:
> I review latest patchset.

Thanks, I have committed the refactoring of src/common/ as a first
step.

> I have one big question: Is pg_checksums
> safe for cross-versions operations? Even with update_controlfile
> call? Currently i am able to enable checksums in pg11 cluster with
> pg_checksums compiled on HEAD. Is this expected? I didn't notice any
> version-specific check in code.

This depends on the version of the control file, and it happens that
we don't check for it, so that's a good catch from your side.  Not
doing the check is a bad idea as ControlFileData should be compatible
between the binary and the data read.  I am attaching a fresh 0001
which should be back-patched down to v11 as a bug fix.  An advantage
of that, which is similar to pg_rewind, is that if the control file
version does not change in a major version, then the tool can be
used.  And the data folder layer is unlikely going to change.. 

>> <command>pg_checksums</command> checks, enables or disables data checksums
> 
> maybe better is <application>, not <command>?

Fixed, as part of the renaming patch.

>> +	printf(_("%s enables, disables or verifies data checksums in a PostgreSQL\n"), progname);
>> +	printf(_("database cluster.\n\n"));
> 
> I doubt this is good line formatting for translation purposes.
>
>> +	printf(_("  -c, --check            check data checksums.  This is the default\n"));
>> +	printf(_("                         mode if nothing is specified.\n"));
> 
> same. For example pg_basebackup uses different multiline style:

Oh, good points.  I forgot about that point of view.

>> +command_fails(['pg_checksums', '--enable', '-r', '1234', '-D', $pgdata],
>> +	      "fails when relfilnodes are requested and action is --disable");
> 
> action is "--enable" here ;-)

s/relfilnodes/relfilenodes/ while on it.

>> if (badblocks > 0)gi
>>	return 1;
> 
> Small question: why return 1 instead of exit(1)?

OK, let's fix that on the way as part of the renaming.

>> <refentry id="pgchecksums">
>>  <indexterm zone="pgchecksums">
> 
> How about use "app-pgchecksums" similar to other applications?

Yes, I was wondering about that one when doing the renaming, but did
not bother much for consistency.  Anyway switched, you are right.

Attached is an updated patch set, minus the refactoring for
src/common/.
--
Michael