Re: Offline enabling/disabling of data checksums
От | Fabien COELHO |
---|---|
Тема | Re: Offline enabling/disabling of data checksums |
Дата | |
Msg-id | alpine.DEB.2.21.1902270720410.10851@lancre обсуждение исходный текст |
Ответ на | Re: Offline enabling/disabling of data checksums (Michael Banck <michael.banck@credativ.de>) |
Ответы |
Re: Offline enabling/disabling of data checksums
Re: Offline enabling/disabling of data checksums |
Список | 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.
В списке pgsql-hackers по дате отправления: