On Fri, Feb 17, 2017 at 2:28 AM, David Christensen <david@endpoint.com> wrote:
> - Change "data_checksums" from a simple boolean to "data_checksum_state", an enum type for all of
> the potentially-required states for this feature (as well as enabling).
Color me skeptical. I don't know what CHECKSUMS_ENABLING,
CHECKSUMS_ENFORCING, and CHECKSUMS_REVALIDATING are intended to
represent -- and there's no comments in the patch explaining it -- but
if we haven't yet written the code to enable checksums, how do we know
for sure which states it will require?
If we're going to accept a patch to disable checksums without also
having the capability to enable checksums, I think we should leave out
the speculative elements about what might be needed on the "enable"
side and just implement the minimal "disable" side.
However, FWIW, I don't accept that being able to disable checksums
online is a sufficient advance to justify enabling checksums by
default. Tomas had some good points on another thread about what
might be needed to really make that a good choice, and I'm still
skeptical about whether checksums catch any meaningful number of
errors that wouldn't be caught otherwise, and about the degree to
which any complaints it issues are actionable. I'm not really against
this patch on its own merits, but I think it's a small advance in an
area that needs a lot of work. I think it would be a lot more useful
if we had a way to *enable* checksums online. Then people who find
out that checksums exist and want them have an easier way of getting
them, and anyone who uses the functionality in this patch and then
regrets it has a way to get back.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company