Offline enabling/disabling of data checksums

classic Classic list List threaded Threaded
145 messages Options
1234 ... 8
Reply | Threaded
Open this post in threaded view
|

Offline enabling/disabling of data checksums

Michael Banck-2
Hi,

the attached patch adds offline enabling/disabling of checksums to
pg_verify_checksums. It is based on independent work both Michael
(Paquier) and me did earlier this year and takes changes from both, see
https://github.com/credativ/pg_checksums and
https://github.com/michaelpq/pg_plugins/tree/master/pg_checksums

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

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?

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.

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

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).


Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: [hidden email]

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

offline-activation-of-checksums_V1.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Banck-2
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: [hidden email]

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

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Robert Haas
In reply to this post by Michael Paquier-2
On Fri, Dec 21, 2018 at 6:28 PM Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3
In reply to this post by Michael Banck-2

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.
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3

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.
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Magnus Hagander-2
In reply to this post by Michael Paquier-2


On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <[hidden email]> 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. 

--
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Magnus Hagander-2
In reply to this post by Michael Paquier-2


On Sat, Dec 22, 2018 at 12:28 AM Michael Paquier <[hidden email]> 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)

--
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3
In reply to this post by Magnus Hagander-2

>> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Tomas Vondra-4
In reply to this post by Magnus Hagander-2
On 12/27/18 11:43 AM, Magnus Hagander wrote:

>
>
> On Sat, Dec 22, 2018 at 12:28 AM Michael Paquier <[hidden email]
> <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Tomas Vondra-4
In reply to this post by Magnus Hagander-2


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

>
>
> On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <[hidden email]
> <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Magnus Hagander-2


On Thu, Dec 27, 2018 at 3:54 PM Tomas Vondra <[hidden email]> wrote:


On 12/27/18 11:39 AM, Magnus Hagander wrote:
>
>
> On Thu, Dec 27, 2018 at 2:15 AM Michael Paquier <[hidden email]
> <mailto:[hidden email]>> 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". 

--
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Tomas Vondra-4
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Tomas Vondra-4


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

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Magnus Hagander-2
In reply to this post by Tomas Vondra-4


On Fri, Dec 28, 2018 at 1:14 AM Tomas Vondra <[hidden email]> 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. 

--
Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Fabien COELHO-3

>>> [...]
>>
>> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Offline enabling/disabling of data checksums

Michael Paquier-2
In reply to this post by Magnus Hagander-2
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

signature.asc (849 bytes) Download Attachment
1234 ... 8