Thread: Enable data checksums by default

Hi,

On Tue, Aug 06, 2024 at 06:46:52PM -0400, Greg Sabino Mullane wrote:
> Please find attached a patch to enable data checksums by default.
> 
> Currently, initdb only enables data checksums if passed the
> --data-checksums or -k argument. There was some hesitation years ago when
> this feature was first added, leading to the current situation where the
> default is off. However, many years later, there is wide consensus that
> this is an extraordinarily safe, desirable setting. Indeed, most (if not
> all) of the major commercial and open source Postgres systems currently
> turn this on by default. I posit you would be hard-pressed to find many
> systems these days in which it has NOT been turned on. So basically we have
> a de-facto standard, and I think it's time we flipped the switch to make it
> on by default.

[...]
 
> Yes, I am aware of the previous discussions on this, but the world moves
> fast - wal compression is better than in the past, vacuum is better now,
> and data-checksums being on is such a complete default in the wild, it
> feels weird and a disservice that we are not running all our tests like
> that.

I agree.

Some review on the patch:

> diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
> index bdd613e77f..511f489d34 100644
> --- a/doc/src/sgml/ref/initdb.sgml
> +++ b/doc/src/sgml/ref/initdb.sgml
> @@ -267,12 +267,14 @@ PostgreSQL documentation
>         <para>
>          Use checksums on data pages to help detect corruption by the
>          I/O system that would otherwise be silent. Enabling checksums
> -        may incur a noticeable performance penalty. If set, checksums
> +        may incur a small performance penalty. If set, checksums
>          are calculated for all objects, in all databases. All checksum

I think the last time we dicussed this the consensus was that
computational overhead of computing the checksums is pretty small for
most systems (so the above change seems warranted regardless of whether
we switch the default), but turning on wal_compression also turns on
wal_log_hints, which can increase WAL by quite a lot. Maybe this is
covered elsewhere in the documentation (I just looked at the patch), but
if not, it probably should be added here as a word of caution.

>          failures will be reported in the
>          <link linkend="monitoring-pg-stat-database-view">
>          <structname>pg_stat_database</structname></link> view.
>          See <xref linkend="checksums" /> for details.
> +        As of version 18, checksums are enabled by default. They can be
> +        disabled by use of <option>--no-data-checksums</option>.

I think we usually do not mention when a feature was added/changed, do
we? So I'd just write "(default: enabled)" or whatever is the style of
the surrounding options.

>         </para>
>        </listitem>
>       </varlistentry>
> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
> index f00718a015..ce7d3e99e5 100644
> --- a/src/bin/initdb/initdb.c
> +++ b/src/bin/initdb/initdb.c
> @@ -164,7 +164,7 @@ static bool noinstructions = false;
>  static bool do_sync = true;
>  static bool sync_only = false;
>  static bool show_setting = false;
> -static bool data_checksums = false;
> +static bool data_checksums = true;
>  static char *xlog_dir = NULL;
>  static int    wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024);
>  static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC;
> @@ -3121,6 +3121,7 @@ main(int argc, char *argv[])
>          {"waldir", required_argument, NULL, 'X'},
>          {"wal-segsize", required_argument, NULL, 12},
>          {"data-checksums", no_argument, NULL, 'k'},
> +        {"no-data-checksums", no_argument, NULL, 20},

Does it make sense to add -K (capital k) as a short-cut for this? I
think this is how we distinguish on/off for pg_dump (-t/-T etc.) but
maybe that is not wider project policy.


Michael

On 07.08.24 00:46, Greg Sabino Mullane wrote:
> Currently, initdb only enables data checksums if passed the 
> --data-checksums or -k argument. There was some hesitation years ago 
> when this feature was first added, leading to the current situation 
> where the default is off. However, many years later, there is wide 
> consensus that this is an extraordinarily safe, desirable setting. 
> Indeed, most (if not all) of the major commercial and open source 
> Postgres systems currently turn this on by default. I posit you would be 
> hard-pressed to find many systems these days in which it has NOT been 
> turned on. So basically we have a de-facto standard, and I think it's 
> time we flipped the switch to make it on by default.

I'm sympathetic to this proposal, but I want to raise some concerns.

My understanding was that the reason for some hesitation about adopting 
data checksums was the performance impact.  Not the checksumming itself, 
but the overhead from hint bit logging.  The last time I looked into 
that, you could get performance impacts on the order of 5% tps.  Maybe 
that's acceptable, and you of course can turn it off if you want the 
extra performance.  But I think this should be discussed in this thread.

About the claim that it's already the de-facto standard.  Maybe that is 
approximately true for "serious" installations.  But AFAICT, the popular 
packagings don't enable checksums by default, so there is likely a 
significant middle tier between "just trying it out" and serious 
production use that don't have it turned on.

For those uses, this change would render pg_upgrade useless for upgrades 
from an old instance with default settings to a new instance with 
default settings.  And then users would either need to re-initdb with 
checksums turned back off, or I suppose run pg_checksums on the old 
instance before upgrading?  This is significant additional complication. 
  And packagers who have built abstractions on top of pg_upgrade (such 
as Debian pg_upgradecluster) would also need to implement something to 
manage this somehow.

So I think we need to think through the upgrade experience a bit more. 
Unfortunately, pg_checksums hasn't gotten to the point that we were 
perhaps once hoping for that you could enable checksums on a live 
system.  I'm thinking pg_upgrade could have a mode where it adds the 
checksum during the upgrade as it copies the files (essentially a subset 
of pg_checksums).  I think that would be useful for that middle tier of 
users who just want a good default experience.

On Thu, Aug 08, 2024 at 12:11:38PM +0200, Peter Eisentraut wrote:
> So I think we need to think through the upgrade experience a bit more.
> Unfortunately, pg_checksums hasn't gotten to the point that we were perhaps
> once hoping for that you could enable checksums on a live system.  I'm
> thinking pg_upgrade could have a mode where it adds the checksum during the
> upgrade as it copies the files (essentially a subset of pg_checksums).  I
> think that would be useful for that middle tier of users who just want a
> good default experience.

Well that, or, as a first less ambitious step, pg_upgrade could carry
over the data_checksums setting from the old to the new instance by
essentially disabling it via pg_checksums -d (which is fast) if it the
current default (off) is set on the old instance and the new instance
was created with the new onw (checksums on).

Probably should include a warning or something in that case, though I
guess a lot of users will read just past it. But at least they are not
worse off than before.


Michael

> On 8 Aug 2024, at 12:11, Peter Eisentraut <peter@eisentraut.org> wrote:

> My understanding was that the reason for some hesitation about adopting data checksums was the performance impact.
Notthe checksumming itself, but the overhead from hint bit logging.  The last time I looked into that, you could get
performanceimpacts on the order of 5% tps.  Maybe that's acceptable, and you of course can turn it off if you want the
extraperformance.  But I think this should be discussed in this thread. 

That's been my experience as well, the overhead of the checksumming is
negligible but the overhead in WAL can be (having hint bits WAL logged does
carry other benefits as well to be fair).

> I think we need to think through the upgrade experience a bit more.

+1

> Unfortunately, pg_checksums hasn't gotten to the point that we were perhaps once hoping for that you could enable
checksumson a live system.   

I don't recall there being any work done (or plans for) using pg_checksums on a
live system.  Anyone interested in enabling checksums on a live cluster can
however review the patch for that in:

  https://postgr.es/m/E07A611B-9CF3-4FDB-8CE8-A221E39040EC@yesql.se

> I'm thinking pg_upgrade could have a mode where it adds the checksum during the upgrade as it copies the files
(essentiallya subset of pg_checksums).  I think that would be useful for that middle tier of users who just want a good
defaultexperience. 

As a side-note, I implemented this in pg_upgrade at Greenplum (IIRC it was
submitted to -hackers at the time as well) and it worked well with not a lot of
code.

--
Daniel Gustafsson

On Thu, Aug 8, 2024 at 6:11 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> About the claim that it's already the de-facto standard.  Maybe that is
> approximately true for "serious" installations.  But AFAICT, the popular
> packagings don't enable checksums by default, so there is likely a
> significant middle tier between "just trying it out" and serious
> production use that don't have it turned on.

+1.

> I'm thinking pg_upgrade could have a mode where it adds the
> checksum during the upgrade as it copies the files (essentially a subset
> of pg_checksums).  I think that would be useful for that middle tier of
> users who just want a good default experience.

That would be very nice.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 8/8/24 19:42, Robert Haas wrote:
> On Thu, Aug 8, 2024 at 6:11 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> About the claim that it's already the de-facto standard.  Maybe that is
>> approximately true for "serious" installations.  But AFAICT, the popular
>> packagings don't enable checksums by default, so there is likely a
>> significant middle tier between "just trying it out" and serious
>> production use that don't have it turned on.
> 
> +1.
> 
>> I'm thinking pg_upgrade could have a mode where it adds the
>> checksum during the upgrade as it copies the files (essentially a subset
>> of pg_checksums).  I think that would be useful for that middle tier of
>> users who just want a good default experience.
> 
> That would be very nice.
> 

Yeah, but it might also disable checksums on the new cluster, which
would work for link mode too. So we'd probably want multiple modes, one
to enable checksums during file copy, one to disable checksums, and one
to just fail for incompatible clusters.


-- 
Tomas Vondra

On Tue, Aug 13, 2024 at 10:42 AM Greg Sabino Mullane <htamfids@gmail.com> wrote:
> Fair enough. I think the performance impact is acceptable, as evidenced by the large number of people that turn it
on.And it is easy enough to turn it off again, either via --no-data-checksums or pg_checksums --disable. 
> When I did some measurements some time ago, I found numbers much less than 5%, but of course it depends on a lot of
factors.

I think the bad case is when you have a write workload that is
significantly bigger than shared_buffers but still small enough to fit
comfortably in the OS cache. When everything fits in shared_buffers,
you only need to write dirty buffers once per checkpoint cycle, so
making it more expensive isn't necessarily a big deal. When you're
constantly going to disk, that's so expensive that you don't notice
the computational overhead. But when you're in that middle zone where
you keep evicting buffers from PG but not actually having to write
them down to the disk, then I think it's pretty noticeable.

> I've come across people who have regretted not throwing a -k into their initial initdb, but have not yet come across
someonewho has the opposite regret. 

I don't think this is really a fair comparison, because everything
being a little slower all the time is not something that people are
likely to "regret" in the same way that they regret it when a data
corruption issue goes undetected. An undetected data corruption issue
is a single, very painful event that people are likely to notice,
whereas a small performance loss over time kind of blends into the
background. You don't really regret that kind of thing in the same way
that you regret a bad event that happens at a particular moment in
time.

And it's not like we have statistics anywhere that you can look at to
see how much CPU time you spent computing checksums, so if a user DOES
have a performance problem that would not have occurred if checksums
had been disabled, they'll probably never know it.

>> For those uses, this change would render pg_upgrade useless for upgrades from an old instance with default settings
toa new instance with default settings.  And then users would either need to re-initdb with checksums turned back off,
orI suppose run pg_checksums on the old instance before upgrading?  This is significant additional complication. 
> Meh, re-running initdb with --no-data-checksums seems a fairly low hurdle.

I tend to agree with that, but I would also like to see the sort of
improvements that Peter mentions. It's a lot less work to say "let's
just change the default" and then get mad at anyone who disagrees than
it is to do the engineering to make changing the default less of a
problem. But that kind of engineering really adds a lot of value
compared to just changing the default.

None of that is to say that I'm totally hostile to this change.
Checksums don't actually prevent your data from getting corrupted, or
let you recover it after it does. They just tell you about the
problem, and very often you would have found out anyway. However, they
do have peace-of-mind value. If you've got checksums turned on, you
can verify your checksums regularly and see that they're OK, and
people like that. Whether that's worth the overhead for everyone, I'm
not quite sure.

--
Robert Haas
EDB: http://www.enterprisedb.com