Offline enabling/disabling of data checksums

On Fri, Dec 21, 2018 at 09:16:16PM +0100, Michael Banck wrote:

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

There are two discussion points which deserve attention here:
1) Do we want to rename pg_verify_checksums to something else, like
pg_checksums. I like a lot if we would do a simple renaming of the
tool, which should be the first step taken.
2) Which kind of interface do we want to use? When I did my own
flavor of pg_checksums, I used an --action switch able to use the
following values:
- enable
- disable
- verify
The switch cannot be specified twice (perhaps we could enforce the
last value as other binaries do in the tree, not sure if that's
adapted here). A second type of interface is to use one switch per
action. For both interfaces if no action is specify then the tool
fails. Vote is open.

This is basically meant as a stop-gap measure in case online activation
of checksums won't make it for v12, but maybe it is independently
useful?

I think that this is independently useful, I got this stuff part of an
upgrade workflow where the user is ready to accept some extra one-time
offline time so as checksums are enabled.

Things I have not done so far:

1. Rename pg_verify_checksums to e.g. pg_checksums as it will no longer
only verify checksums.

Check. That sounds right to me.

2. Rename the scan_* functions (Michael renamed them to operate_file and
operate_directory but I am not sure it is worth it.

The renaming makes sense, as scan implies only reading while enabling
checksums causes a write.

3. Once that patch is in, there would be a way to disable checksums so
there'd be a case to also change the initdb default to enabled, but that
required further discussion (and maybe another round of benchmarks).

Perhaps, that's unrelated to this thread though. I am not sure that
all users would be ready to pay the extra cost of checksums enabled by
default.
--
Michael

Hi,

I have added it to the commitfest now:

https://commitfest.postgresql.org/21/1944/

On Sat, Dec 22, 2018 at 08:28:34AM +0900, Michael Paquier wrote:

On Fri, Dec 21, 2018 at 09:16:16PM +0100, Michael Banck wrote:

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

There are two discussion points which deserve attention here:
1) Do we want to rename pg_verify_checksums to something else, like
pg_checksums. I like a lot if we would do a simple renaming of the
tool, which should be the first step taken.

I am for it, but don't mind whether it's before or afterwards, your
call.

2) Which kind of interface do we want to use? When I did my own
flavor of pg_checksums, I used an --action switch able to use the
following values:
- enable
- disable
- verify
The switch cannot be specified twice (perhaps we could enforce the
last value as other binaries do in the tree, not sure if that's
adapted here). A second type of interface is to use one switch per
action. For both interfaces if no action is specify then the tool
fails. Vote is open.

Even though my fork has the separate switches, I like the --action one.
On the other hand, it is a bit more typing as you always have to spell
out the action (is there precendent of accepting also incomplete option
arguments like 'v', 'e', 'd'?).

This is basically meant as a stop-gap measure in case online activation
of checksums won't make it for v12, but maybe it is independently
useful?

I think that this is independently useful, I got this stuff part of an
upgrade workflow where the user is ready to accept some extra one-time
offline time so as checksums are enabled.

OK; we have also used that at clients - if the instance has a size of
less than a few dozen GBs, enabling checksums during a routine minor
upgrade restart is not delaying things much.

2. Rename the scan_* functions (Michael renamed them to operate_file and
operate_directory but I am not sure it is worth it.

The renaming makes sense, as scan implies only reading while enabling
checksums causes a write.

Ok, will do in the next version.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

On Sat, Dec 22, 2018 at 02:42:55PM +0100, Michael Banck wrote:

On Sat, Dec 22, 2018 at 08:28:34AM +0900, Michael Paquier wrote:

There are two discussion points which deserve attention here:
1) Do we want to rename pg_verify_checksums to something else, like
pg_checksums. I like a lot if we would do a simple renaming of the
tool, which should be the first step taken.

I am for it, but don't mind whether it's before or afterwards, your
call.

Doing the renaming after would be a bit weird logically, as we would
finish with a point in time in the tree where pg_verify_checksums is
able to do something else than just verifying checksums.

Even though my fork has the separate switches, I like the --action one.
On the other hand, it is a bit more typing as you always have to spell
out the action (is there precendent of accepting also incomplete option
arguments like 'v', 'e', 'd'?).

Yes, there is a bit of that in psql for example for formats. Not sure
that we should take this road for a checksumming tool though. If a
new option is added which takes the first letter then we would have
incompatibility issues. That's unlikely to happen, still that feels
uneasy.
--
Michael

On Fri, Dec 21, 2018 at 6:28 PM Michael Paquier <michael@paquier.xyz> wrote:

2) Which kind of interface do we want to use? When I did my own
flavor of pg_checksums, I used an --action switch able to use the
following values:
- enable
- disable
- verify
The switch cannot be specified twice (perhaps we could enforce the
last value as other binaries do in the tree, not sure if that's
adapted here). A second type of interface is to use one switch per
action. For both interfaces if no action is specify then the tool
fails. Vote is open.

I vote for separate switches. Using the same switch with an argument
seems like it adds typing for no real gain.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hallo Michael,

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable, and verify
would be the default if none is provided.

This is basically meant as a stop-gap measure in case online activation
of checksums won't make it for v12, but maybe it is independently
useful?

I would say yes.

1. Rename pg_verify_checksums to e.g. pg_checksums as it will no longer
only verify checksums.

I'd agree to rename the tool as "pg_checksums".

2. Rename the scan_* functions (Michael renamed them to operate_file and
operate_directory but I am not sure it is worth it.

Hmmm. The file is indeed scanned, and "operate" is kind of very fuzzy.

3. Once that patch is in, there would be a way to disable checksums so
there'd be a case to also change the initdb default to enabled, but that
required further discussion (and maybe another round of benchmarks).

My 0.02ￜ is that data safety should comes first, thus checksums should be
enabled by default.

About the patch: applies, compiles, "make check" ok.

There is no documentation.

In "scan_file", I would open RW only for enable, but keep RO for verify.

Also, the full page is rewritten... would it make sense to only overwrite
the checksum part itself?

It seems that the control file is unlinked and then rewritten. If the
rewritting fails, or the command is interrupted, the user has a problem.

Could the control file be simply opened RW? Else, I would suggest to
rename (eg add .tmp), write the new one, then unlink the old one, so that
recovering the old state in case of problem is possible.

For enable/disable, while the command is running, it should mark the
cluster as opened to prevent an unwanted database start. I do not see
where this is done.

--
Fabien.

On Wed, Dec 26, 2018 at 07:43:17PM +0100, Fabien COELHO wrote:

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable, and verify
would be the default if none is provided.

Okay, noted for the separate switches. But I don't agree with the
point of assuming that --verify should be enforced if no switches are
defined. That feels like a trap for newcomers of this tool..

For enable/disable, while the command is running, it should mark the cluster
as opened to prevent an unwanted database start. I do not see where this is
done.

You have pretty much the same class of problems if you attempt to
start a cluster on which pg_rewind or the existing pg_verify_checksums
is run after these have scanned the control file to make sure that
they work on a cleanly-stopped instance. In short, this is a deal
between code simplicity and trying to have the tool outsmart users in
the way users use the tool. I tend to prefer keeping the code simple
and not worry about cases where the users mess up with their
application, as there are many more things which could go wrong.
--
Michael

Bonjour Michaï¿œl,

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable, and verify
would be the default if none is provided.

Okay, noted for the separate switches. But I don't agree with the
point of assuming that --verify should be enforced if no switches are
defined. That feels like a trap for newcomers of this tool..

Hmmm. It does something safe and useful, especially if it also works
online (patch pending), and the initial tool only does checking. However,
I'd be okay for no default.

For enable/disable, while the command is running, it should mark the cluster
as opened to prevent an unwanted database start. I do not see where this is
done.

You have pretty much the same class of problems if you attempt to
start a cluster on which pg_rewind or the existing pg_verify_checksums
is run after these have scanned the control file to make sure that
they work on a cleanly-stopped instance. In short, this is a deal
between code simplicity and trying to have the tool outsmart users in
the way users use the tool. I tend to prefer keeping the code simple
and not worry about cases where the users mess up with their
application, as there are many more things which could go wrong.

Hmmm. I do not buy the comparison.

A verify that fails is not a big problem, you can run it again. If
pg_rewind fails, you can probably run it again as well, the source is
probably still consistent even if it has changed, and too bad for the
target side, but it was scheduled to be overwritten anyway.

However, a tool which overwrites files beyond the back of a running server
is a recipee for data-loss, so I think it should take much more care, i.e.
set the server state into some specific safe state.

About code simplicity: probably there is, or there should be, a
change-the-state function somewhere, because quite a few tools could use
it?

--
Fabien.

On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 26, 2018 at 07:43:17PM +0100, Fabien COELHO wrote:

It adds an (now mandatory) --action parameter that takes either verify,
enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable, and

verify

would be the default if none is provided.

Okay, noted for the separate switches. But I don't agree with the
point of assuming that --verify should be enforced if no switches are
defined. That feels like a trap for newcomers of this tool..

Defaulting to the choice that makes no actual changes to the data surely is
the safe choice,a nd not a trap :)

That said, this would probably be our first tool where you switch it
between readonly and rewrite mode with just a switch, woudn't it? All other
tools are either read-only or read/write at the *tool* level, not the
switch level.

That in itself would be an argument for making it a separate tool. But not
a very strong one I think, I prefer the single-tool-renamed approach as
well.

There's plenty enough precedent for the "separate switches and a default
behaviour if none is specified" in other tools though, and I don't think
that's generally considered a trap.

So count me in the camp for separate switches and default to verify. If one
didn't mean that, it's only a quick Ctrl-C away with no damage done.

For enable/disable, while the command is running, it should mark the
cluster

as opened to prevent an unwanted database start. I do not see where this

is

done.

You have pretty much the same class of problems if you attempt to
start a cluster on which pg_rewind or the existing pg_verify_checksums
is run after these have scanned the control file to make sure that
they work on a cleanly-stopped instance. In short, this is a deal
between code simplicity and trying to have the tool outsmart users in
the way users use the tool. I tend to prefer keeping the code simple
and not worry about cases where the users mess up with their
application, as there are many more things which could go wrong.

I think it comes down to what the outcome is. If we're going to end up with
a corrupt database (e.g. one where checksums aren't set everywhere but they
are marked as such in pg_control) then it's not acceptable. If the only
outcome is the tool gives an error that's not an error and if re-run it's
fine, then it's a different story.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Sat, Dec 22, 2018 at 12:28 AM Michael Paquier <michael@paquier.xyz>
wrote:

On Fri, Dec 21, 2018 at 09:16:16PM +0100, Michael Banck wrote:

I think that this is independently useful, I got this stuff part of an
upgrade workflow where the user is ready to accept some extra one-time
offline time so as checksums are enabled.

Very much so, IMHO.

Things I have not done so far:

1. Rename pg_verify_checksums to e.g. pg_checksums as it will no longer
only verify checksums.

Check. That sounds right to me.

Should we double-check with packagers that this won't cause a problem?
Though the fact that it's done in a major release should make it perfectly
fine I think -- and it's a smaller change than when we did all those
xlog->wal changes...

3. Once that patch is in, there would be a way to disable checksums so

there'd be a case to also change the initdb default to enabled, but that
required further discussion (and maybe another round of benchmarks).

Perhaps, that's unrelated to this thread though. I am not sure that
all users would be ready to pay the extra cost of checksums enabled by
default.

I'd be a strong +1 for changing the default once we have a painless way to
turn them off.

It remains super-cheap to turn them off (stop database, one command, turn
them on). So those people that aren't willing to pay the overhead of
checksums, can very cheaply get away from it.

It's a lot more expensive to turn them on once your database has grown to
some size (definitely in offline mode, but also in an online mode when we
get that one in).

Plus, the majority of people *should* want them on :) We don't run with say
synchronous_commit=off by default either to make it easier on those that
don't want to pay the overhead of full data safety :P (I know it's not a
direct match, but you get the idea)

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

For enable/disable, while the command is running, it should mark the
cluster as opened to prevent an unwanted database start. I do not see
where this is done.

You have pretty much the same class of problems if you attempt to
start a cluster on which pg_rewind or the existing pg_verify_checksums
is run after these have scanned the control file to make sure that
they work on a cleanly-stopped instance. [...]

I think it comes down to what the outcome is. If we're going to end up with
a corrupt database (e.g. one where checksums aren't set everywhere but they
are marked as such in pg_control) then it's not acceptable. If the only
outcome is the tool gives an error that's not an error and if re-run it's
fine, then it's a different story.

ISTM that such an outcome is indeed a risk, as a starting postgres could
update already checksummed pages without putting a checksum. It could be
even worse, although with a (very) low probability, with updates
overwritten on a race condition between the processes. In any case, no
error would be reported before much later, with invalid checksums or
inconsistent data, or undetected forgotten committed data.

--
Fabien.

On 12/27/18 11:43 AM, Magnus Hagander wrote:

On Sat, Dec 22, 2018 at 12:28 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:

On Fri, Dec 21, 2018 at 09:16:16PM +0100, Michael Banck wrote:

I think that this is independently useful, I got this stuff part of an
upgrade workflow where the user is ready to accept some extra one-time
offline time so as checksums are enabled.

Very much so, IMHO.

Things I have not done so far:

1. Rename pg_verify_checksums to e.g. pg_checksums as it will no

longer

only verify checksums.

Check.  That sounds right to me.

Should we double-check with packagers that this won't cause a problem?
Though the fact that it's done in a major release should make it
perfectly fine I think -- and it's a smaller change than when we did all
those xlog->wal changes...

I think it makes little sense to not rename the tool now. I'm pretty
sure we'd end up doing that sooner or later anyway, and we'll just live
with a misnamed tool until then.

3. Once that patch is in, there would be a way to disable checksums so
there'd be a case to also change the initdb default to enabled,

but that

required further discussion (and maybe another round of benchmarks).

Perhaps, that's unrelated to this thread though.  I am not sure that
all users would be ready to pay the extra cost of checksums enabled by
default.

I'd be a strong +1 for changing the default once we have a painless way
to turn them off.

It remains super-cheap to turn them off (stop database, one command,
turn them on). So those people that aren't willing to pay the overhead
of checksums, can very cheaply get away from it.

It's a lot more expensive to turn them on once your database has grown
to some size (definitely in offline mode, but also in an online mode
when we get that one in).

Plus, the majority of people *should* want them on :) We don't run with
say synchronous_commit=off by default either to make it easier on those
that don't want to pay the overhead of full data safety :P (I know it's
not a direct match, but you get the idea)

I don't know, TBH. I agree making the on/off change cheaper makes moves
us closer to 'on' by default, because they may disable it if needed. But
it's not the whole story.

If we enable checksums by default, 99% users will have them enabled.
That means more people will actually observe data corruption cases that
went unnoticed so far. What shall we do with that? We don't have very
good answers to that (tooling, docs) and I'd say "disable checksums" is
not a particularly amazing response in this case :-(

FWIW I don't know what to do about that. We certainly can't prevent the
data corruption, but maybe we could help with fixing it (although that's
bound to be low-level work).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/27/18 11:39 AM, Magnus Hagander wrote:

On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:

On Wed, Dec 26, 2018 at 07:43:17PM +0100, Fabien COELHO wrote:

It adds an (now mandatory) --action parameter that takes either

verify,

enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable,

and verify

would be the default if none is provided.

Okay, noted for the separate switches.  But I don't agree with the
point of assuming that --verify should be enforced if no switches are
defined.  That feels like a trap for newcomers of this tool..

Defaulting to the choice that makes no actual changes to the data surely
is the safe choice,a nd not a trap :)

That said, this would probably be our first tool where you switch it
between readonly and rewrite mode with just a switch, woudn't it? All
other tools are either read-only or read/write at the *tool* level, not
the switch level.

Eh? Isn't pg_rewind "modify by default" with --dry-run switch to run in
a read-only mode. So I'm not sure what you mean by "tool level" here.

FWIW I'd prefer sticking to the same approach for this tool too, i.e.
have a "dry-run" switch that makes it read-only. IMHO that's pretty
common pattern.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Dec 27, 2018 at 3:54 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

On 12/27/18 11:39 AM, Magnus Hagander wrote:

On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <michael@paquier.xyz
<mailto:michael@paquier.xyz>> wrote:

On Wed, Dec 26, 2018 at 07:43:17PM +0100, Fabien COELHO wrote:

It adds an (now mandatory) --action parameter that takes either

verify,

enable or disable as argument.

I'd rather have explicit switches for verify, enable & disable,

and verify

would be the default if none is provided.

Okay, noted for the separate switches. But I don't agree with the
point of assuming that --verify should be enforced if no switches are
defined. That feels like a trap for newcomers of this tool..

Defaulting to the choice that makes no actual changes to the data surely
is the safe choice,a nd not a trap :)

That said, this would probably be our first tool where you switch it
between readonly and rewrite mode with just a switch, woudn't it? All
other tools are either read-only or read/write at the *tool* level, not
the switch level.

Eh? Isn't pg_rewind "modify by default" with --dry-run switch to run in
a read-only mode. So I'm not sure what you mean by "tool level" here.

FWIW I'd prefer sticking to the same approach for this tool too, i.e.
have a "dry-run" switch that makes it read-only. IMHO that's pretty
common pattern.

That's a different thing.

pg_rewind in dry-run mode does the same thing, except it doesn't actually
do it, it just pretends.

Verifying checksums is not the same as "turn on checksums except don't
actually do it" or "turn off checksums except don't actually do it".

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Thu, Dec 27, 2018 at 03:46:48PM +0100, Tomas Vondra wrote:

On 12/27/18 11:43 AM, Magnus Hagander wrote:

Should we double-check with packagers that this won't cause a problem?
Though the fact that it's done in a major release should make it
perfectly fine I think -- and it's a smaller change than when we did all
those xlog->wal changes...

I think it makes little sense to not rename the tool now. I'm pretty
sure we'd end up doing that sooner or later anyway, and we'll just live
with a misnamed tool until then.

Do you think that a thread Would on -packagers be more adapted then?

I don't know, TBH. I agree making the on/off change cheaper makes moves
us closer to 'on' by default, because they may disable it if needed. But
it's not the whole story.

If we enable checksums by default, 99% users will have them enabled.
That means more people will actually observe data corruption cases that
went unnoticed so far. What shall we do with that? We don't have very
good answers to that (tooling, docs) and I'd say "disable checksums" is
not a particularly amazing response in this case :-(

Enabling data checksums by default is still a couple of steps ahead,
without a way to control them better..

FWIW I don't know what to do about that. We certainly can't prevent the
data corruption, but maybe we could help with fixing it (although that's
bound to be low-level work).

Yes, data checksums are extremely useful to tell people when the
problem is *not* from Postgres, which can be really hard in a large
organization. Knowing about the corrupted page is also useful as you
can look at its contents and look at its bytes before it gets zero'ed
to spot patterns which can help other teams in charge of a lower level
of the application layer.
--
Michael

On 12/28/18 12:25 AM, Michael Paquier wrote:

On Thu, Dec 27, 2018 at 03:46:48PM +0100, Tomas Vondra wrote:

On 12/27/18 11:43 AM, Magnus Hagander wrote:

Should we double-check with packagers that this won't cause a problem?
Though the fact that it's done in a major release should make it
perfectly fine I think -- and it's a smaller change than when we did all
those xlog->wal changes...

I think it makes little sense to not rename the tool now. I'm pretty
sure we'd end up doing that sooner or later anyway, and we'll just live
with a misnamed tool until then.

Do you think that a thread Would on -packagers be more adapted then?

I'm sorry, but I'm not sure I understand the question. Of course, asking
over at -packagers won't hurt, but my guess is the response will be it's
not a big deal from the packaging perspective.

I don't know, TBH. I agree making the on/off change cheaper makes moves
us closer to 'on' by default, because they may disable it if needed. But
it's not the whole story.

If we enable checksums by default, 99% users will have them enabled.
That means more people will actually observe data corruption cases that
went unnoticed so far. What shall we do with that? We don't have very
good answers to that (tooling, docs) and I'd say "disable checksums" is
not a particularly amazing response in this case :-(

Enabling data checksums by default is still a couple of steps ahead,
without a way to control them better..

What do you mean by "control" here? Dealing with checksum failures, or
some additional capabilities?

FWIW I don't know what to do about that. We certainly can't prevent the
data corruption, but maybe we could help with fixing it (although that's
bound to be low-level work).

Yes, data checksums are extremely useful to tell people when the
problem is *not* from Postgres, which can be really hard in a large
organization. Knowing about the corrupted page is also useful as you
can look at its contents and look at its bytes before it gets zero'ed
to spot patterns which can help other teams in charge of a lower level
of the application layer.

I'm not sure data checksums are particularly great evidence. For example
with the recent fsync issues, we might have ended with partial writes
(and thus invalid checksums). The OS migh have even told us about the
failure, but we've gracefully ignored it. So I'm afraid data checksums
are not a particularly great proof it's not our fault.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 28, 2018 at 01:14:05AM +0100, Tomas Vondra wrote:

I'm sorry, but I'm not sure I understand the question. Of course, asking
over at -packagers won't hurt, but my guess is the response will be it's
not a big deal from the packaging perspective.

(The previous email had an extra "Would"... Sorry.)
Let's ask those folks then.

What do you mean by "control" here? Dealing with checksum failures, or
some additional capabilities?

What I am referring to here is the possibility to enable, disable and
check checksums for an online cluster. I am not sure what kind of
tooling able to do chirurgy at page level would make sense. Once a
checksum is corrupted a user knows about a problem, which mainly needs
a human lookup.

I'm not sure data checksums are particularly great evidence. For example
with the recent fsync issues, we might have ended with partial writes
(and thus invalid checksums). The OS migh have even told us about the
failure, but we've gracefully ignored it. So I'm afraid data checksums
are not a particularly great proof it's not our fault.

Sure, they are not a solution to all problems. Still they give hints
before the problem spreads, and sometimes by looking at one corrupted
page by yourself one can see if the data fetched from disk comes from
Postgres or not (say inspecting the page header with pageinspect,
etc.).
--
Michael

On Fri, Dec 28, 2018 at 1:14 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

On 12/28/18 12:25 AM, Michael Paquier wrote:

On Thu, Dec 27, 2018 at 03:46:48PM +0100, Tomas Vondra wrote:

On 12/27/18 11:43 AM, Magnus Hagander wrote:

Should we double-check with packagers that this won't cause a problem?
Though the fact that it's done in a major release should make it
perfectly fine I think -- and it's a smaller change than when we did

all

those xlog->wal changes...

I think it makes little sense to not rename the tool now. I'm pretty
sure we'd end up doing that sooner or later anyway, and we'll just live
with a misnamed tool until then.

Do you think that a thread Would on -packagers be more adapted then?

I'm sorry, but I'm not sure I understand the question. Of course, asking
over at -packagers won't hurt, but my guess is the response will be it's
not a big deal from the packaging perspective.

I think a heads- up in the way of "planning to change it, now's the time to
yell" is the reasonable thing.

I don't know, TBH. I agree making the on/off change cheaper makes moves

us closer to 'on' by default, because they may disable it if needed. But
it's not the whole story.

If we enable checksums by default, 99% users will have them enabled.
That means more people will actually observe data corruption cases that
went unnoticed so far. What shall we do with that? We don't have very
good answers to that (tooling, docs) and I'd say "disable checksums" is
not a particularly amazing response in this case :-(

Enabling data checksums by default is still a couple of steps ahead,
without a way to control them better..

What do you mean by "control" here? Dealing with checksum failures, or
some additional capabilities?

FWIW I don't know what to do about that. We certainly can't prevent the
data corruption, but maybe we could help with fixing it (although that's
bound to be low-level work).

Yes, data checksums are extremely useful to tell people when the
problem is *not* from Postgres, which can be really hard in a large
organization. Knowing about the corrupted page is also useful as you
can look at its contents and look at its bytes before it gets zero'ed
to spot patterns which can help other teams in charge of a lower level
of the application layer.

I'm not sure data checksums are particularly great evidence. For example
with the recent fsync issues, we might have ended with partial writes
(and thus invalid checksums). The OS migh have even told us about the
failure, but we've gracefully ignored it. So I'm afraid data checksums
are not a particularly great proof it's not our fault.

They are a great evidence that your data is corrupt. You *want* to know
that your data is corrupt. Even if our best recommendation is "go restore
your backups", you still want to know. Otherwise you are sitting around on
data that's corrupt and you don't know about it.

There are certainly many things we can do to improve the experience. But
not telling people their data is coorrupt when it is, isn't one of them.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

[...]

I'm not sure data checksums are particularly great evidence. For example
with the recent fsync issues, we might have ended with partial writes
(and thus invalid checksums). The OS migh have even told us about the
failure, but we've gracefully ignored it. So I'm afraid data checksums
are not a particularly great proof it's not our fault.

They are a great evidence that your data is corrupt. You *want* to know
that your data is corrupt. Even if our best recommendation is "go restore
your backups", you still want to know. Otherwise you are sitting around on
data that's corrupt and you don't know about it.

There are certainly many things we can do to improve the experience. But
not telling people their data is coorrupt when it is, isn't one of them.

Yep, anyone should want to know if their database is corrupt, compare to
ignoring the fact.

One reason not to enable it could be if the implementation is not trusted,
i.e. if false positive (corrupt page detected while the data are okay and
there was only an issue with computing or storing the checksum) can occur.

There is also the performance impact. I did some quick-and-dirty pgbench
simple update single thread performance tests to compare with vs without
checksum. Enabling checksums on these tests seems to induce a 1.4%
performance penalty, although I'm moderately confident about it given the
standard deviation. At least it is an indication, and it seems to me that
it is consistent with other figures previously reported on the list.

--
Fabien.

On Fri, Dec 28, 2018 at 10:12:24AM +0100, Magnus Hagander wrote:

I think a heads- up in the way of "planning to change it, now's the time to
yell" is the reasonable thing.

And done.
--
Michael