Re: Offline enabling/disabling of data checksums

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

Hello

I review latest patchset. 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.

And few small notes:

> <command>pg_checksums</command> checks, enables or disables data checksums

maybe better is <application>, not <command>?

> + 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:

> printf(_(" -r, --max-rate=RATE maximum transfer rate to transfer data directory\n"
> " (in kB/s, or use suffix \"k\" or \"M\")\n"));

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

action is "--enable" here ;-)

> if (badblocks > 0)
> return 1;

Small question: why return 1 instead of exit(1)?

> <refentry id="pgchecksums">
> <indexterm zone="pgchecksums">

How about use "app-pgchecksums" similar to other applications?

regards, Sergei