From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Michael Banck <michael(dot)banck(at)credativ(dot)de> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Postgres hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Offline enabling/disabling of data checksums |
Date: | 2019-02-27 06:59:31 |
Message-ID: | alpine.DEB.2.21.1902270720410.10851@lancre |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hallo Michael,
>> - * src/bin/pg_verify_checksums/pg_verify_checksums.c
>> + * src/bin/pg_checksums/pg_checksums.c
>> That's lacking a rename, or this comment is incorrect.
>
> Right, I started the rename, but then backed off pending further
> discussion whether I should submit that or whether the committer will
> just do it.
ISTM that there is a all clear for renaming.
The renaming implies quite a few changes (eg in the documentation,
makefiles, tests) which warrants a review, so it should be a patch. Also,
ISTM that the renaming only make sense when adding the enable/disable
feature, so I'd say that it belongs to this patch. Opinions?
About v4:
Patch applies cleanly, compiles, global & local "make check" are ok.
Doc: "enable, disable or verifies" -> "checks, enables or disables"?
Spurious space: "> ." -> ">.".
As checksum are now checked, the doc could use "check" instead of
"verify", especially if there is a rename and the "verify" word
disappears.
I'd be less terse when documenting actions, eg: "Disable checksums" ->
"Disable checksums on cluster."
Doc should state that checking is the default action, eg "Check checksums
on cluster. This is the default action."
Help string could say that -c is the default action. There is a spurious
help line remaining from the previous "--action" implementation.
open: I'm positively unsure about ?: priority over |, and probably not the
only one, so I'd add parentheses around the former.
I'm at odds with the "postmaster.pid" check, which would not prevent an
issue if a cluster is started with "postmaster". I still think that the
enabling-in-progress should be stored in the cluster state.
ISTM that the cluster read/update cycle should lock somehow the control
file being modified. However other commands do not seem to do something
about it.
I do not think that enabling if already enabled or disabling or already
disable should exit(1), I think it is a no-op and should simply exit(0).
About tests: I'd run a check on a disabled cluster to check that the
command fails because disabled.
--
Fabien.
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2019-02-27 07:05:02 | Re: [HACKERS] generated columns |
Previous Message | Yuzuko Hosoya | 2019-02-27 06:50:39 | RE: Problem with default partition pruning |