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_controlfilecall? Currently i am able to enable checksums in pg11 cluster with pg_checksums compiled on HEAD. Is
thisexpected? 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
