Re: Offline enabling/disabling of data checksums - Mailing list pgsql-hackers

From Sergei Kornilov
Subject Re: Offline enabling/disabling of data checksums
Date
Msg-id 155231347133.16480.11453587097036807558.pgcf@coridan.postgresql.org
Whole thread Raw
In response to Re: Offline enabling/disabling of data checksums  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Offline enabling/disabling of data checksums
Re: Offline enabling/disabling of data checksums
List pgsql-hackers
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

pgsql-hackers by date:

Previous
From: Andy Fan
Date:
Subject: Re: Suggestions on message transfer among backends
Next
From: Tom Lane
Date:
Subject: Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance