Re: Enabling Checksums

As I understand it, the main part of the remaining work to be done for
the checksums patch (at least the first commit) is to have a better way
to enable/disable them.

For the sake of simplicity (implementation as well as usability), it
seems like there is agreement that checksums should be enabled or
disabled for the entire instance, not per-table.

I don't think a GUC entirely makes sense (in its current form, anyway).
We basically care about 3 states:
1. Off: checksums are not written, nor are they verified. Pages that
are newly dirtied have the checksum information in the header cleared.
2. Enabling: checksums are written for every dirty page, but only
verified for pages where the checksum is present (as determined by
information in the page header).
3. On: checksums are written for every dirty page, and verified for
every page that's read. If a page does not have a checksum, it's
corrupt.

Does it make sense to store this information in pg_control? That doesn't
require adding any new file, and it has the benefit that it's already
checksummed. It's available during recovery and can be made available
pretty easily in the places where we write data.

And the next question is what commands to add to change state. Ideas:

CHECKSUMS ENABLE; -- set state to "Enabling"
CHECKSUMS DISABLE; -- set state to "Off"

And then to get to the "On" state, you have to run a system-wide VACUUM
while in the "Enabling" state. Or, if the above syntax causes problems,
we can make all of these into VACUUM options.

Thoughts?

Regards,
Jeff Davis

Jeff Davis wrote:

> And the next question is what commands to add to change state. Ideas:
>
> CHECKSUMS ENABLE; -- set state to "Enabling"
> CHECKSUMS DISABLE; -- set state to "Off"
>
> And then to get to the "On" state, you have to run a system-wide VACUUM
> while in the "Enabling" state. Or, if the above syntax causes problems,
> we can make all of these into VACUUM options.

There's no such thing as a system-wide VACUUM. The most you can get is
a database-wide VACUUM, which means you'd have to store the state
per-database somewhere (presumably the pg_database catalog), and perhaps
pg_control could have it as a system-wide value that's computed as the
minimum of all database states (so it stays "enabling" until all
databases have upgraded to "on").

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Friday, November 09, 2012 6:32 AM Jeff Davis wrote:
> As I understand it, the main part of the remaining work to be done for
> the checksums patch (at least the first commit) is to have a better way
> to enable/disable them.
>
> For the sake of simplicity (implementation as well as usability), it
> seems like there is agreement that checksums should be enabled or
> disabled for the entire instance, not per-table.
>
> I don't think a GUC entirely makes sense (in its current form, anyway).
> We basically care about 3 states:
> 1. Off: checksums are not written, nor are they verified. Pages that
> are newly dirtied have the checksum information in the header cleared.
> 2. Enabling: checksums are written for every dirty page, but only
> verified for pages where the checksum is present (as determined by
> information in the page header).
> 3. On: checksums are written for every dirty page, and verified for
> every page that's read. If a page does not have a checksum, it's
> corrupt.
>
> Does it make sense to store this information in pg_control? That doesn't
> require adding any new file, and it has the benefit that it's already
> checksummed. It's available during recovery and can be made available
> pretty easily in the places where we write data.
>
> And the next question is what commands to add to change state. Ideas:
>
> CHECKSUMS ENABLE; -- set state to "Enabling"
> CHECKSUMS DISABLE; -- set state to "Off"
>
> And then to get to the "On" state, you have to run a system-wide VACUUM
> while in the "Enabling" state. Or, if the above syntax causes problems,
> we can make all of these into VACUUM options.

I think one thing may needs to be taken care during such a VACUUM operation
is not to allow user to say
CHECKSUM DISABLE.

Also how about following ways :
1. Allow CHECKSUM Enable only during initdb as mentioned by Robert.
Allow user to only do CHECKSUM DISABLE after initdb.
2. Do the Checksum only for particular pages (SRLU) or to do for System
tables only.

With Regards,
Amit Kapila.

On 09/11/12 02:01, Jeff Davis wrote:
> As I understand it, the main part of the remaining work to be done for
> the checksums patch (at least the first commit) is to have a better way
> to enable/disable them.
>
> For the sake of simplicity (implementation as well as usability), it
> seems like there is agreement that checksums should be enabled or
> disabled for the entire instance, not per-table.
I can definately see that simplicity is an argument here, but
I can easily imagine that some performance hungry users
would prefer to be able to disable the functionality on a
per table level. UNCHECKSUMMED TABLES (similar to UNLOGGED TABLES).

I would definately stuff our system in state = 2 in your
description if it was available.

--
Jesper

Jeff,

On 11/09/2012 02:01 AM, Jeff Davis wrote:
> For the sake of simplicity (implementation as well as usability), it
> seems like there is agreement that checksums should be enabled or
> disabled for the entire instance, not per-table.

Agreed. I've quickly thought about making it a per-database setting, but
how about shared system catalogs... Let's keep it simple and have a
single per-cluster instance switch for now.

> I don't think a GUC entirely makes sense (in its current form, anyway).
> We basically care about 3 states:
> 1. Off: checksums are not written, nor are they verified. Pages that
> are newly dirtied have the checksum information in the header cleared.
> 2. Enabling: checksums are written for every dirty page, but only
> verified for pages where the checksum is present (as determined by
> information in the page header).
> 3. On: checksums are written for every dirty page, and verified for
> every page that's read. If a page does not have a checksum, it's
> corrupt.

Sounds sane, yes.

> And the next question is what commands to add to change state. Ideas:
>
> CHECKSUMS ENABLE; -- set state to "Enabling"
> CHECKSUMS DISABLE; -- set state to "Off"

Yet another SQL command doesn't feel like the right thing for such a
switch. Quick googling revealed that CHECKSUM is a system function in MS
SQL and MySQL knows a CHECKSUM TABLE command. And you never know what
the committee is coming up with next.

Apart from that, I'd like something more descriptive that just
"checksums". Block checksums? Heap checksums? Data checksums?

Regards

Markus Wanner

On 11/09/2012 06:18 AM, Jesper Krogh wrote:
> I would definately stuff our system in state = 2 in your
> description if it was available.

Hm.. that's an interesting statement.

What's probably worst when switching from OFF to ON is the VACUUM run
that needs to touch every page (provided you haven't ever turned
checksumming on before). Maybe you want to save that step and still get
the additional safety for newly dirtied pages, right?

A use case worth supporting?

Regards

Markus Wanner

Jeff,

> I don't think a GUC entirely makes sense (in its current form, anyway).
> We basically care about 3 states:

Huh? Why would a GUC not make sense? How else would you make sure that
checksums where on when you started the system?

> 1. Off: checksums are not written, nor are they verified. Pages that
> are newly dirtied have the checksum information in the header cleared.
> 2. Enabling: checksums are written for every dirty page, but only
> verified for pages where the checksum is present (as determined by
> information in the page header).
> 3. On: checksums are written for every dirty page, and verified for
> every page that's read. If a page does not have a checksum, it's
> corrupt.

Well, large databases would tend to be stuck permanently in "Enabling",
becuase the user would never vacuum old cold partitions in order to
checksum them. So we need to be prepared for this to be the end state
for a lot of databases.

In fact, we'd need three settings for the checksum GUC:

OFF -- don't checksum anything, equal to state (1) above

WRITES -- checksum pages which are being written anyway, but ignore
tables which aren't touched. Permanent "Enabling" state.

ALL -- checksum everything you can. particularly, autovacuum would
checksum any table which was not already checksummed at the next vacuum
of that table. Goal is to get to state 3 above.

> Does it make sense to store this information in pg_control? That doesn't
> require adding any new file, and it has the benefit that it's already
> checksummed. It's available during recovery and can be made available
> pretty easily in the places where we write data.
>
> And the next question is what commands to add to change state. Ideas:
>
> CHECKSUMS ENABLE; -- set state to "Enabling"
> CHECKSUMS DISABLE; -- set state to "Off"

Don't like this, please make it a GUC.

> And then to get to the "On" state, you have to run a system-wide VACUUM
> while in the "Enabling" state. Or, if the above syntax causes problems,
> we can make all of these into VACUUM options.

As there's no such thing as system-wide vacuum, we're going to have to
track whether a table is "fully checksummed" in the system catalogs.
We'll also need:

VACUUM ( CHECKSUM ON )

... which would vacuum an entire table, skipping no pages and writing
checksums for every page, unless the table were marked fully checksummed
already, in which case it would do a regular vacuum.

Once a table was flagged as "all checksummed", then the system could
start producing errors (or warnings?) whenever a page with a missing
checksum was found.

Hmmm, better to have a 2nd GUC:

checksum_fail_action = WARNING | ERROR

... since some people want the write or read to fail, and others just
want to see it in the logs.

So, thinking about it, state (3) is never the state of an entire
installation; it's always the state of individual tables.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, 2012-11-09 at 15:42 +0100, Markus Wanner wrote:
> On 11/09/2012 06:18 AM, Jesper Krogh wrote:
> > I would definately stuff our system in state = 2 in your
> > description if it was available.
>
> Hm.. that's an interesting statement.
>
> What's probably worst when switching from OFF to ON is the VACUUM run
> that needs to touch every page (provided you haven't ever turned
> checksumming on before). Maybe you want to save that step and still get
> the additional safety for newly dirtied pages, right?
>
> A use case worth supporting?

One problem is telling which pages are protected and which aren't. We
can have a couple bits in the header indicating that a checksum is
present, but it's a little disappointing to have only a few bits
protecting a 16-bit checksum.

Also, I think that people will want to have a way to protect their old
data somehow.

Regards,
Jeff Davis

On Thu, 2012-11-08 at 23:33 -0300, Alvaro Herrera wrote:
> There's no such thing as a system-wide VACUUM. The most you can get is
> a database-wide VACUUM, which means you'd have to store the state
> per-database somewhere (presumably the pg_database catalog), and perhaps
> pg_control could have it as a system-wide value that's computed as the
> minimum of all database states (so it stays "enabling" until all
> databases have upgraded to "on").

That's a good point. Maybe this should be done as an offline operation
using a command-line utility?

Regards,
Jeff Davis

On 11/09/2012 07:53 PM, Jeff Davis wrote:
> One problem is telling which pages are protected and which aren't. We
> can have a couple bits in the header indicating that a checksum is
> present, but it's a little disappointing to have only a few bits
> protecting a 16-bit checksum.

Given your description of option 2 I was under the impression that each
page already has a bit indicating whether or not the page is protected
by a checksum. Why do you need more bits than that?

> Also, I think that people will want to have a way to protect their old
> data somehow.

Well, given that specific set of users is not willing to go through a
rewrite of each and every page of its database, it's hard to see how we
can protect their old data better.

However, we certainly need to provide the option to go through the
rewrite for other users, who are well willing to bite that bullet.

From a users perspective, the trade-off seems to be: if you want your
old data to be covered by checksums, you need to go through such an
expensive VACUUM run that touches every page in your database.

If you don't want to or cannot do that, you can still turn on
checksumming for newly written pages. You won't get full protection and
it's hard to tell what data is protected and what not, but it's still
better than no checksumming at all. Especially for huge databases, that
might be a reasonable compromise.

One could even argue, that this just leads to a prolonged migration and
with time, the remaining VACUUM step becomes less and less frightening.

Do you see any real foot-guns or other show-stoppers for permanently
allowing that in-between-state?

Or do we have other viable options that prolong the migration and thus
spread the load better over time?

Regards

Markus Wanner

On Fri, 2012-11-09 at 20:48 +0100, Markus Wanner wrote:
> Given your description of option 2 I was under the impression that each
> page already has a bit indicating whether or not the page is protected
> by a checksum. Why do you need more bits than that?

The bit indicating that a checksum is present may be lost due to
corruption.

> However, we certainly need to provide the option to go through the
> rewrite for other users, who are well willing to bite that bullet.

That's the use case that I've been focusing on, but perhaps you are
right that it's not the only important one.

> Do you see any real foot-guns or other show-stoppers for permanently
> allowing that in-between-state?

The biggest problem that I see is a few bits indicating the presence of
a checksum may be vulnerable to more kinds of corruption.

Regards,
Jeff Davis

On Nov10, 2012, at 00:08 , Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Fri, 2012-11-09 at 20:48 +0100, Markus Wanner wrote:
>> Given your description of option 2 I was under the impression that each
>> page already has a bit indicating whether or not the page is protected
>> by a checksum. Why do you need more bits than that?
>
> The bit indicating that a checksum is present may be lost due to
> corruption.

Though that concern mostly goes away if instead of a separate bit we use a
special checksum value, say 0xDEAD, to indicate that the page isn't
checksummed, no?

If checksums were always enabled, the probability of a random corruption
going undetected is N/N^2 = 1/N where N is the number of distinct checksum
values, since out of the N^2 equally likely pairs of computed and stored
checksums values, N show two identical values.

With the 0xDEAD-scheme, the probability of a random corruption going
undetected is (N-1 + N)/N^2 = 2/N - 1/N^2, since there are (N-1) pairs
with identical values != 0xDEAD, and N pairs where the stored checksum
value is 0xDEAD.

So instead of a 1 in 65536 chance of a corruption going undetected, the
0xDEAD-schema gives (approximately) a chance of 1 in 32768, i.e the
strength of the checksum is reduced by one bit. That's still acceptable,
I'd say.

In practice, 0xDEAD may be a bad choice because of it's widespread use
as an uninitialized marker for blocks of memory. A randomly picked value
would probably be a better choice.

best regards,
Florian Pflug

On Sat, 2012-11-10 at 14:46 +0100, Florian Pflug wrote:
> > The bit indicating that a checksum is present may be lost due to
> > corruption.
>
> Though that concern mostly goes away if instead of a separate bit we use a
> special checksum value, say 0xDEAD, to indicate that the page isn't
> checksummed, no?

Right. But then we have an upgrade impact to set the checksum to 0xDEAD
on all existing pages, which seems to eliminate most of the possible
reason for it.

Also, we'd need to tweak the algorithm to make sure that it never landed
on that magic value. So if we think we might want this in the future, we
should reserve that magic value now.

But I can't think of many reasons for it, unless we expect people to be
turning checksums on and off repeatedly.

Regards,
Jeff Davis

On Fri, 2012-11-09 at 09:57 -0800, Josh Berkus wrote:
> Huh? Why would a GUC not make sense? How else would you make sure that
> checksums where on when you started the system?

If we stored the information in pg_control, you could check with
pg_controldata. We could have a separate utility, pg_checksums, that can
alter the state and/or do an offline verification. And initdb would take
an option that would start everything out fully protected with
checksums.

The problem with a GUC is that checksums aren't really something you can
change by just changing the variable and restarting, unless you are only
using checksums opportunistically (only write checksums when a page is
dirtied and only verify a checksum if the header indicates that it's
present).

There are also usability issues. If someone has a fully-protected
instance, and turns the GUC off, and starts the server, they'll lose the
"fully-protected" status on the first write, and have to re-read all the
data to get back to fully protected. That just doesn't seem right to me.

> Well, large databases would tend to be stuck permanently in "Enabling",
> becuase the user would never vacuum old cold partitions in order to
> checksum them. So we need to be prepared for this to be the end state
> for a lot of databases.

That may be true, but if that's the case, it's more like a 3-bit
checksum than a 16-bit checksum, because of the page-header corruption
problem. I don't know of any way to give those users more than that,
which won't be good enough for the set-at-initdb time users.

> In fact, we'd need three settings for the checksum GUC:
>
> OFF -- don't checksum anything, equal to state (1) above
>
> WRITES -- checksum pages which are being written anyway, but ignore
> tables which aren't touched. Permanent "Enabling" state.
>
> ALL -- checksum everything you can. particularly, autovacuum would
> checksum any table which was not already checksummed at the next vacuum
> of that table. Goal is to get to state 3 above.

That's slightly more eager, but it's basically the same as the WRITES
state. In order to get to the fully-protected state, you still need to
somehow make sure that all of the old data is checksummed.

And the "fully protected" state is important in my opinion, because
otherwise we aren't protected against corrupt page headers that say they
have no checksum (even when it really should have a checksum).

> > Does it make sense to store this information in pg_control? That doesn't
> > require adding any new file, and it has the benefit that it's already
> > checksummed. It's available during recovery and can be made available
> > pretty easily in the places where we write data.
> >
> > And the next question is what commands to add to change state. Ideas:
> >
> > CHECKSUMS ENABLE; -- set state to "Enabling"
> > CHECKSUMS DISABLE; -- set state to "Off"
>
> Don't like this, please make it a GUC.

I'll see if you have ideas about how to resolve the problems with a GUC
that I mentioned above. But if not, then what about using a utility,
perhaps called pg_checksums? That way we wouldn't need new syntax.

> As there's no such thing as system-wide vacuum, we're going to have to
> track whether a table is "fully checksummed" in the system catalogs.

It seems like this is going down the road of per-table checksums. I'm
not opposed to that, but that has a low chance of making 9.3.

Let's try to do something simpler now that leaves open the possibility
of more flexibility later. I'm inclined to agree with Robert that the
first patch should probably be an initdb-time option. Then, we can allow
a lazy mode (like your WRITES state) and an eager offline check with a
pg_checksums utility. Then we can work towards per-table checksums,
control via VACUUM, protecting the SLRU, treating zero pages as invalid,
protecting temp files (which can be a GUC), replication integration,
etc.

> Hmmm, better to have a 2nd GUC:
>
> checksum_fail_action = WARNING | ERROR
>
> ... since some people want the write or read to fail, and others just
> want to see it in the logs.

Checksums don't introduce new failure modes on writes, only on reads.

And for reads, I think we have a problem doing anything less than an
ERROR. If we allow the read to succeed, we either risk a crash (or
silently corrupting other buffers in shared memory), or we have to put a
zero page in its place. But we already have the zero_damaged_pages
option, which I think is better because reading corrupt data is only
useful for data recovery efforts.

> So, thinking about it, state (3) is never the state of an entire
> installation; it's always the state of individual tables.

That contradicts the idea of using a GUC then. It would make more sense
to have extra syntax or extra VACUUM modes to accomplish that per-table.

Unfortunately, I'm worried that the per-table approach will not be
completed by 9.3. Do you see something about my proposal that makes it
harder to get where we want to go in the future?

If we do ultimately get per-table checksums, then I agree that a flag in
pg_control may be a bit of a wart, but it's easy enough to remove later.

Regards,
Jeff Davis

Hello

>
>> > Does it make sense to store this information in pg_control? That doesn't
>> > require adding any new file, and it has the benefit that it's already
>> > checksummed. It's available during recovery and can be made available
>> > pretty easily in the places where we write data.
>> >
>> > And the next question is what commands to add to change state. Ideas:
>> >
>> > CHECKSUMS ENABLE; -- set state to "Enabling"
>> > CHECKSUMS DISABLE; -- set state to "Off"
>>
>> Don't like this, please make it a GUC.
>
> I'll see if you have ideas about how to resolve the problems with a GUC
> that I mentioned above. But if not, then what about using a utility,
> perhaps called pg_checksums? That way we wouldn't need new syntax.

I don't think so GUC are good for this purpouse, but I don't like
single purpouse statements too.

what do you think about enhancing ALTER DATABASE statement

some like

ALTER DATABASE name ENABLE CHECKSUMS and ALTER DATABASE name DISABLE CHECKSUMS

Regards

Pavel

On Sun, 2012-11-11 at 21:20 +0100, Pavel Stehule wrote:
> I don't think so GUC are good for this purpouse, but I don't like
> single purpouse statements too.
>
> what do you think about enhancing ALTER DATABASE statement
>
> some like
>
> ALTER DATABASE name ENABLE CHECKSUMS and ALTER DATABASE name DISABLE CHECKSUMS

Per-database does sound easier than per-table. I'd have to think about
how that would affect shared catalogs though.

For now, I'm leaning toward an offline utility to turn checksums on or
off, called pg_checksums. It could do so lazily (just flip a switch to
"enabling" in pg_control), or it could do so eagerly and turn it into a
fully-protected instance.

For the first patch, it might just be an initdb-time option for
simplicity.

Regards,
Jeff Davis

On 11/11/2012 05:52 PM, Jeff Davis wrote:
> On Sun, 2012-11-11 at 21:20 +0100, Pavel Stehule wrote:
>> I don't think so GUC are good for this purpouse, but I don't like
>> single purpouse statements too.
>>
>> what do you think about enhancing ALTER DATABASE statement
>>
>> some like
>>
>> ALTER DATABASE name ENABLE CHECKSUMS and ALTER DATABASE name DISABLE CHECKSUMS
> Per-database does sound easier than per-table. I'd have to think about
> how that would affect shared catalogs though.
>
> For now, I'm leaning toward an offline utility to turn checksums on or
> off, called pg_checksums. It could do so lazily (just flip a switch to
> "enabling" in pg_control), or it could do so eagerly and turn it into a
> fully-protected instance.
>
> For the first patch, it might just be an initdb-time option for
> simplicity.
>

+1

I haven't followed this too closely, but I did wonder several days ago
why this wasn't being made an initdb-time decision.

cheers

andrew

On 11/11/12 2:56 PM, Jeff Davis wrote:
> We could have a separate utility, pg_checksums, that can
> alter the state and/or do an offline verification. And initdb would take
> an option that would start everything out fully protected with
> checksums.

Adding an initdb option to start out with everything checksummed seems
an uncontroversial good first thing to have available. It seems like a
proper 9.3 target to aim at even if per-table upgrading gets bogged down
in details. I have an argument below that the area between initdb and
per-table upgrades is fundamentally uncertain and therefore not worth
chasing after, based on reasons you already started to outline. There's
not much useful middle ground there.

Won't a pg_checksums program just grow until it looks like a limited
version of vacuum though? It's going to iterate over most of the table;
it needs the same cost controls as autovacuum (and to respect the load
of concurrent autovacuum work) to keep I/O under control; and those cost
control values might change if there's a SIGHUP to reload parameters.
It looks so much like vacuum that I think there needs to be a really
compelling reason to split it into something new. Why can't this be yet
another autovacuum worker that does its thing?

> In order to get to the fully-protected state, you still need to
> somehow make sure that all of the old data is checksummed.
>
> And the "fully protected" state is important in my opinion, because
> otherwise we aren't protected against corrupt page headers that say
> they have no checksum (even when it really should have a checksum).

I think it's useful to step back for a minute and consider the larger
uncertainty an existing relation has, which amplifies just how ugly this
situation is. The best guarantee I think online checksumming can offer
is to tell the user "after transaction id X, all new data in relation R
is known to be checksummed". Unless you do this at initdb time, any
conversion case is going to have the possibility that a page is
corrupted before you get to it--whether you're adding the checksum as
part of a "let's add them while we're writing anyway" page update or the
conversion tool is hitting it.

That's why I don't think anyone will find online conversion really
useful until they've done a full sweep updating the old pages. And if
you accept that, a flexible checksum upgrade utility, one that co-exists
with autovacuum activity costs, becomes a must.

One of the really common cases I was expecting here is that conversions
are done by kicking off a slow background VACUUM CHECKSUM job that might
run in pieces. I was thinking of an approach like this:

-Initialize a last_checked_block value for each table
-Loop:
--Grab the next block after the last checked one
--When on the last block of the relation, grab an exclusive lock to
protect against race conditions with extension
--If it's marked as checksummed and the checksum matches, skip it
---Otherwise, add a checksum and write it out
--When that succeeds, update last_checked_block
--If that was the last block, save some state saying the whole table is
checkedsummed

With that logic, there is at least a forward moving pointer that removes
the uncertainty around whether pages have been updated or not. It will
keep going usefully if interrupted too. One obvious this way this can
fail is if:

1) A late page in the relation is updated and a checksummed page written
2) The page is corrupted such that the "is this checksummed?" bits are
not consistent anymore, along with other damage to it
3) The conversion process gets to this page eventually
4) The corruption of (2) isn't detected

But I think that this possibility--that a page might get quietly
corrupted after checked once, but still in the middle of checking a
relation--is both impossible to remove and a red herring. How do we
know that this page of the relation wasn't corrupted on disk before we
even started? We don't, and we can't.

The only guarantee I see that we can give for online upgrades is that
after a VACUUM CHECKSUM sweep is done, and every page is known to both
have a valid checksum on it and have its checksum bits set, *then* any
page that doesn't have both set bits and a matching checksum is garbage.
Until reaching that point, any old data is suspect. The idea of
operating in an "we'll convert on write but never convert old pages"
can't come up with any useful guarantees about data integrity that I can
see. As you say, you don't ever gain the ability to tell pages that
were checksummed but have since been corrupted from ones that were
corrupt all along in that path.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 12/11/12 05:55, Greg Smith wrote:
> The only guarantee I see that we can give for online upgrades is that
> after a VACUUM CHECKSUM sweep is done, and every page is known to both
> have a valid checksum on it and have its checksum bits set, *then* any
> page that doesn't have both set bits and a matching checksum is
> garbage. Until reaching that point, any old data is suspect. The
> idea of operating in an "we'll convert on write but never convert old
> pages" can't come up with any useful guarantees about data integrity
> that I can see. As you say, you don't ever gain the ability to tell
> pages that were checksummed but have since been corrupted from ones
> that were corrupt all along in that path.

You're right about that, but I'd just like some rough guard against
hardware/OS related data corruption.
and that is more likely to hit data-blocks constantly flying in and out
of the system.
I'm currently running a +2TB database and the capabillity to just see
some kind of corruption earlier
rather than later is a major benefit by itself. Currently corruption can
go undetected if it just
happens to hit data-only parts of the database.

But I totally agree that the scheme described with integrating it into a
autovacuum process would
be very close to ideal, even on a database as the one I'l running.

--
Jesper

On 11/12/12 12:55 AM, Jesper Krogh wrote:
> I'd just like some rough guard against
> hardware/OS related data corruption.
> and that is more likely to hit data-blocks constantly flying in and out
> of the system.

I get that. I think that some of the design ideas floating around since
this feature was first proposed have been innovating in the hope of
finding a clever halfway point here. Ideally we'd be able to get online
checksum conversion and up running easily, reliably, and without adding
a lot of code. I have given up on that now though.

The approach of doing a heavy per-table conversion with more state
information than we'd like seems unavoidable, if you want to do it right
and allow people to (slowly but surely) reach a trustworthy state. I
think we should stop searching for a clever way around and just do slog
through doing it. I've resigned myself to that now, and recently set
aside a good block of time to beat my head against that particular wall
over the next couple of months.

> But I totally agree that the scheme described with integrating it into a
> autovacuum process would
> be very close to ideal, even on a database as the one I'm running.

I am sadly all too familiar with how challenging it is to keep a 2TB
PostgreSQL database running reliably. One of my recent catch phrases
for talks is "if you have a big Postgres database, you also have a
vacuum problem". I think it's unreasonable to consider online
conversion solutions that don't recognize that, and allow coordinating
the work with the challenges of vacuuming larger systems too.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

Jeff,

On 11/10/2012 12:08 AM, Jeff Davis wrote:
> The bit indicating that a checksum is present may be lost due to
> corruption.

Hm.. I see.

Sorry if that has been discussed before, but can't we do without that
bit at all? It adds a checksum switch to each page, where we just agreed
we don't event want a per-database switch.

Can we simply write a progress indicator to pg_control or someplace
saying that all pages up to X of relation Y are supposed to have valid
checksums?

That would mean having to re-calculate the checksums on pages that got
dirtied before VACUUM came along to migrate them to having a checksum,
but that seems acceptable. VACUUM could even detect that case and
wouldn't have to re-write it with the same contents.

I realize this doesn't support Jesper's use case of wanting to have the
checksums only for newly dirtied pages. However, I'd argue that
prolonging the migration to spread the load would allow even big shops
to go through this without much of an impact on performance.

Regards

Markus Wanner

On 11/12/2012 05:55 AM, Greg Smith wrote:
> Adding an initdb option to start out with everything checksummed seems
> an uncontroversial good first thing to have available.

+1

So the following discussion really is for a future patch extending on
that initial checkpoint support.

> One of the really common cases I was expecting here is that conversions
> are done by kicking off a slow background VACUUM CHECKSUM job that might
> run in pieces. I was thinking of an approach like this:
>
> -Initialize a last_checked_block value for each table
> -Loop:
> --Grab the next block after the last checked one
> --When on the last block of the relation, grab an exclusive lock to
> protect against race conditions with extension
> --If it's marked as checksummed and the checksum matches, skip it
> ---Otherwise, add a checksum and write it out
> --When that succeeds, update last_checked_block
> --If that was the last block, save some state saying the whole table is
> checkedsummed

Perfect, thanks. That's the rough idea I had in mind as well, written
out in detail and catching the extension case.

> With that logic, there is at least a forward moving pointer that removes
> the uncertainty around whether pages have been updated or not. It will
> keep going usefully if interrupted too. One obvious this way this can
> fail is if:
>
> 1) A late page in the relation is updated and a checksummed page written
> 2) The page is corrupted such that the "is this checksummed?" bits are
> not consistent anymore, along with other damage to it
> 3) The conversion process gets to this page eventually
> 4) The corruption of (2) isn't detected

IMO this just outlines how limited the use of the "is this checksummed"
bit in the page itself is. It just doesn't catch all cases. Is it worth
having that bit at all, given your block-wise approach above?

It really only serves to catch corruptions to *newly* dirtied pages
*during* the migration phase that *keep* that single bit set. Everything
else is covered by the last_checked_block variable. Sounds narrow enough
to be negligible. Then again, it's just a single bit per page...

> The only guarantee I see that we can give for online upgrades is that
> after a VACUUM CHECKSUM sweep is done, and every page is known to both
> have a valid checksum on it and have its checksum bits set, *then* any
> page that doesn't have both set bits and a matching checksum is garbage.

From that point in time on, we'd theoretically better use that bit as an
additional checksum bit rather than requiring it to be set all times.
Really just theoretically, I'm certainly not advocating a 33 bit
checksum :-)

Regards

Markus Wanner

On 11/12/2012 04:44 PM, Markus Wanner wrote:
> Jeff,
>
> On 11/10/2012 12:08 AM, Jeff Davis wrote:
>> The bit indicating that a checksum is present may be lost due to
>> corruption.
> Hm.. I see.
>
> Sorry if that has been discussed before, but can't we do without that
> bit at all? It adds a checksum switch to each page, where we just agreed
> we don't event want a per-database switch.
>
> Can we simply write a progress indicator to pg_control or someplace
> saying that all pages up to X of relation Y are supposed to have valid
> checksums?
That'll make it hard for VACUUM, hint-bit setting, etc to
opportunistically checksum pages whenever they're doing a page write anyway.

Is it absurd to suggest using another bitmap, like the FSM or visibility
map, to store information on page checksumming while checksumming is
enabled but incomplete? As a much smaller file the bitmap could its self
be very quickly generated in one pass when checksumming is enabled, with
its starting state showing no pages having checksums.

It perhaps its self have page checksums since presumably the persistent
maps like the FSM and visibility map will support them? Some way to
ensure the checksum map is valid would be needed.

--
Craig Ringer

On 11/12/2012 10:44 AM, Craig Ringer wrote:
> That'll make it hard for VACUUM, hint-bit setting, etc to
> opportunistically checksum pages whenever they're doing a page write anyway.

It *is* a hard problem, yes. And the single bit doesn't really solve it.
So I'm arguing against opportunistically checksumming in general. Who
needs that anyway?

> Is it absurd to suggest using another bitmap, like the FSM or visibility
> map, to store information on page checksumming while checksumming is
> enabled but incomplete?

Not absurd. But arguably inefficient, because that bitmap may well
become a bottleneck itself. Plus there's the problem of making sure
those pages are safe against corruptions, so you'd need to checksum the
checksum bitmap... doesn't sound like a nice solution to me.

This has certainly been discussed before.

Regards

Markus Wanner

Greg Smith wrote:
> On 11/11/12 2:56 PM, Jeff Davis wrote:
> >We could have a separate utility, pg_checksums, that can
> >alter the state and/or do an offline verification. And initdb would take
> >an option that would start everything out fully protected with
> >checksums.
>
> Adding an initdb option to start out with everything checksummed
> seems an uncontroversial good first thing to have available.

+1

> Won't a pg_checksums program just grow until it looks like a limited
> version of vacuum though? It's going to iterate over most of the
> table; it needs the same cost controls as autovacuum (and to respect
> the load of concurrent autovacuum work) to keep I/O under control;
> and those cost control values might change if there's a SIGHUP to
> reload parameters. It looks so much like vacuum that I think there
> needs to be a really compelling reason to split it into something
> new. Why can't this be yet another autovacuum worker that does its
> thing?

I agree that much of the things it's gonna do are going to be pretty
much the same as vacuum, but vacuum does so many other things that I
think it should be kept separate. Sure, we can make it be invoked from
autovacuum in background according to some (yet to be devised)
scheduling heuristics. But I don't see that it needs to share any
vacuum code.

A couple of thoughts about autovacuum: it's important to figure out
whether checksumming can run concurrently with vacuuming the same table;
if not, which one defers to the other in case of lock conflict. Also,
can checksumming be ignored by concurrent transactions when computing
Xmin (I don't see any reason not to ...)

> One of the really common cases I was expecting here is that
> conversions are done by kicking off a slow background VACUUM
> CHECKSUM job that might run in pieces. I was thinking of an
> approach like this:
>
> -Initialize a last_checked_block value for each table
> -Loop:
> --Grab the next block after the last checked one
> --When on the last block of the relation, grab an exclusive lock to
> protect against race conditions with extension

Note that we have a separate lock type for relation extension, so we can
use that to avoid a conflict here.

> --If it's marked as checksummed and the checksum matches, skip it
> ---Otherwise, add a checksum and write it out
> --When that succeeds, update last_checked_block
> --If that was the last block, save some state saying the whole table
> is checkedsummed

"Some state" can be a pg_class field that's updated per
heap_inplace_update.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 2012-11-12 at 09:44 +0100, Markus Wanner wrote:
> Can we simply write a progress indicator to pg_control or someplace
> saying that all pages up to X of relation Y are supposed to have valid
> checksums?

pg_control would not be the right place for that structure. It's
intended to be fixed-size (it's just a serialized C structure) and it
should be smaller than a sector so that it doesn't suffer from torn
pages.

Not a bad approach overall, but requires some kind of new structure. And
that increases the risk that it doesn't make 9.3.

Right now, I'm honestly just trying to get the simplest approach that
doesn't restrict these kinds of ideas if we want to do them later.

Regards,
Jeff Davis

On Sun, 2012-11-11 at 23:55 -0500, Greg Smith wrote:
> Adding an initdb option to start out with everything checksummed seems
> an uncontroversial good first thing to have available.

OK, so here's my proposal for a first patch (changes from Simon's
patch):

* Add a flag to the postgres executable indicating that it should use
checksums on everything. This would only be valid if bootstrap mode is
also specified.
* Add a multi-state checksums flag in pg_control, that would have
three states: OFF, ENABLING, and ON. It would only be set to ON during
bootstrap, and in this first patch, it would not be possible to set
ENABLING.
* Remove GUC and use this checksums flag everywhere.
* Use the TLI field rather than the version field of the page header.
* Incorporate page number into checksum calculation (already done).

Does this satisfy the requirements for a first step? Does it interfere
with potential future work?

> Won't a pg_checksums program just grow until it looks like a limited
> version of vacuum though?

We can dig into the details of that later, but I don't think it's
useless, even if we do have per-table (or better) checksums. For
instance, it would be useful to verify backups offline.

I think it's a legitimate concern that we might reinvent some VACUUM
machinery. Ideally, we'd get better online migration tools for checksums
(perhaps using VACUUM) fast enough that nobody will bother introducing
that kind of bloat into pg_checksums.

> I think it's useful to step back for a minute and consider the larger
> uncertainty an existing relation has, which amplifies just how ugly this
> situation is. The best guarantee I think online checksumming can offer
> is to tell the user "after transaction id X, all new data in relation R
> is known to be checksummed".

It's slightly better than that. It's more like: "we can tell you if any
of your data gets corrupted after transaction X". If old data is
corrupted before transaction X, then there's nothing we can do. But if
it's corrupted after transaction X (even if it's old data), the
checksums should catch it.

> Unless you do this at initdb time, any
> conversion case is going to have the possibility that a page is
> corrupted before you get to it--whether you're adding the checksum as
> part of a "let's add them while we're writing anyway" page update or the
> conversion tool is hitting it.

Good point.

> That's why I don't think anyone will find online conversion really
> useful until they've done a full sweep updating the old pages.

I don't entirely agree. A lot of times, you just want to know whether
your disk is changing your data out from under you. Maybe you miss some
cases and maybe not all of your data is protected, but just knowing
which disks need to be replaced, and which RAID controllers not to buy
again, is quite valuable. And the more data you get checksummed the
faster you'll find out.

> One of the really common cases I was expecting here is that conversions
> are done by kicking off a slow background VACUUM CHECKSUM job that might
> run in pieces.

Right now I'm focused on the initial patch and other fairly immediate
goals, so I won't address this now. But I don't want to cut off the
conversation, either.

Regards,
Jeff Davis

Jeff,

On 11/12/2012 06:52 PM, Jeff Davis wrote:
> OK, so here's my proposal for a first patch (changes from Simon's
> patch):
>
> * Add a flag to the postgres executable indicating that it should use
> checksums on everything. This would only be valid if bootstrap mode is
> also specified.
> * Add a multi-state checksums flag in pg_control, that would have
> three states: OFF, ENABLING, and ON. It would only be set to ON during
> bootstrap, and in this first patch, it would not be possible to set
> ENABLING.
> * Remove GUC and use this checksums flag everywhere.
> * Use the TLI field rather than the version field of the page header.
> * Incorporate page number into checksum calculation (already done).
>
> Does this satisfy the requirements for a first step? Does it interfere
> with potential future work?

As described before in this thread, I think we might be able to do
without the "has checksum"-bit, as yet another simplification. But I
don't object to adding it, either.

> It's slightly better than that. It's more like: "we can tell you if any
> of your data gets corrupted after transaction X". If old data is
> corrupted before transaction X, then there's nothing we can do. But if
> it's corrupted after transaction X (even if it's old data), the
> checksums should catch it.

I (mis?)read that as Greg referring to the intermediate (enabling)
state, where pages with old data may or may not have a checksum, yet. So
I think it was an argument against staying in that state any longer than
necessary.

Regards

Markus Wanner

On 11/12/12 4:44 AM, Craig Ringer wrote:

> Is it absurd to suggest using another bitmap, like the FSM or visibility
> map, to store information on page checksumming while checksumming is
> enabled but incomplete?

I spent some time thinking about that last week. One problem with it is
that the bitmap structure itself has the same issues as every other
write here--how do we know it's going to disk accurately? The "put
'checksum on' bits on the page" idea and "put checksum on bits in a map"
have the same fundamental issue. Things might get out of sync in the
same way, you've just moved the potentially suspicious write to a new place.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Mon, 2012-11-12 at 20:44 +0100, Markus Wanner wrote:
> As described before in this thread, I think we might be able to do
> without the "has checksum"-bit, as yet another simplification. But I
> don't object to adding it, either.

I see. For a first patch, I guess that's OK. Might as well make it as
simple as possible. We probably need to decide what to do there before
9.3 is released though.

Regards,
Jeff Davis

On 11/12/12 3:44 AM, Markus Wanner wrote:
> Sorry if that has been discussed before, but can't we do without that
> bit at all? It adds a checksum switch to each page, where we just agreed
> we don't event want a per-database switch.

Once you accept that eventually there need to be online conversion
tools, there needs to be some easy way to distinguish which pages have
been processed for several potential implementations. The options seem
to be adding some bits just for that or bumping the page format. I
would like to just bump the format, but that has a pile of its own
issues to cross. Rather not make that a requirement for this month's
requirements.

> Can we simply write a progress indicator to pg_control or someplace
> saying that all pages up to X of relation Y are supposed to have valid
> checksums?

All of the table-based checksum enabling ideas seem destined to add
metadata to pg_class or something related to it for this purpose. While
I think everyone agrees that this is a secondary priority to getting
basic cluster-level checksums going right now, I'd like to have at least
a prototype for that before 9.3 development ends. All of the

> I realize this doesn't support Jesper's use case of wanting to have the
> checksums only for newly dirtied pages. However, I'd argue that
> prolonging the migration to spread the load would allow even big shops
> to go through this without much of an impact on performance.

I'm thinking of this in some ways like the way creation of a new (but
not yet valid) foreign key works. Once that's active, new activity is
immediately protected moving forward. And eventually there's this
cleanup step needed, one that you can inch forward over a few days.

The main upper limit on load spreading here is that the conversion
program may need to grab a snapshot. In that case the conversion taking
too long will be a problem, as it blocks other vacuum activity past that
point. This is why I think any good solution to this problem needs to
incorporate restartable conversion. We were just getting complaints
recently about how losing a CREATE INDEX CONCURRENTLY session can cause
the whole process to end and need to be started over. The way
autovacuum runs right now it can be stopped and restarted later, with
only a small loss of duplicated work in many common cases. If it's
possible to maintain that property for the checksum conversion, that
would be very helpful to larger sites. It doesn't matter if adding
checksums to the old data takes a week if you throttle the load down, so
long as you're not forced to hold an open snapshot the whole time.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

Jeff,

> OK, so here's my proposal for a first patch (changes from Simon's
> patch):
>
> * Add a flag to the postgres executable indicating that it should use
> checksums on everything. This would only be valid if bootstrap mode is
> also specified.
> * Add a multi-state checksums flag in pg_control, that would have
> three states: OFF, ENABLING, and ON. It would only be set to ON during
> bootstrap, and in this first patch, it would not be possible to set
> ENABLING.
> * Remove GUC and use this checksums flag everywhere.
> * Use the TLI field rather than the version field of the page header.
> * Incorporate page number into checksum calculation (already done).
>
> Does this satisfy the requirements for a first step? Does it interfere
> with potential future work?

So the idea of this implementation is that checksums is something you
set at initdb time, and if you want checksums on an existing database,
it's a migration process (e.g. dump and reload)?

I think that's valid as a first cut at this.

We'll need interruptable VACUUM CHECKSUM later, but we don't have to
have it for the first version of the feature.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 11/13/2012 01:22 AM, Greg Smith wrote:
> Once you accept that eventually there need to be online conversion
> tools, there needs to be some easy way to distinguish which pages have
> been processed for several potential implementations.

Agreed. What I'm saying is that this identification doesn't need to be
as fine grained as a per-page bit. A single "horizon" or "border" is
enough, given an ordering of relations (for example by OID) and an
ordering of pages in the relations (obvious).

> All of the table-based checksum enabling ideas ...

This is not really one - it doesn't allow per-table switching. It's just
meant to be a more compact way of representing which pages have been
checksummed and which not.

> I'm thinking of this in some ways like the way creation of a new (but
> not yet valid) foreign key works. Once that's active, new activity is
> immediately protected moving forward. And eventually there's this
> cleanup step needed, one that you can inch forward over a few days.

I understand that. However, I question if users really care. If a
corruption is detected, the clever DBA tells his trainee immediately
check the file- and disk subsystem - no matter whether the corruption
was on old or new data.

You have a point in that pages with "newer" data are often more likely
to be re-read and thus getting checked. Where as the checksums written
to pages with old data might not be re-read any time soon. Starting to
write checksums from the end of the relation could mitigate this to some
extent, though.

Also keep in mind the "quietly corrupted after checked once, but still
in the middle of checking a relation" case. Thus a single bit doesn't
really give us the guarantee you ask for. Sure, we can add more than one
bit. And yeah, if done properly, adding more bits exponentially reduces
the likeliness of a corruption inadvertently turning off checksumming
for a page.

All that said, I'm not opposed to using a few bits of the page header. I
wanted to outline an alternative that I think is viable and less intrusive.

> This is why I think any good solution to this problem needs to
> incorporate restartable conversion.

I fully agree to that.

Regards

Markus Wanner

On Sun, Nov 11, 2012 at 5:52 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Per-database does sound easier than per-table. I'd have to think about
> how that would affect shared catalogs though.
>
> For now, I'm leaning toward an offline utility to turn checksums on or
> off, called pg_checksums. It could do so lazily (just flip a switch to
> "enabling" in pg_control), or it could do so eagerly and turn it into a
> fully-protected instance.
>
> For the first patch, it might just be an initdb-time option for
> simplicity.

It'd be pretty easy to write a pg_checksums utilitys to turn checksums
on/off on a database that is shut down, since the hard part of all of
this is to change the state while the database is running. But I
think even that doesn't need to be part of the first patch. A small
patch that gets committed is better than a big one that doesn't.

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

On Mon, Nov 12, 2012 at 4:44 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> That'll make it hard for VACUUM, hint-bit setting, etc to
> opportunistically checksum pages whenever they're doing a page write anyway.
>
> Is it absurd to suggest using another bitmap, like the FSM or visibility
> map, to store information on page checksumming while checksumming is
> enabled but incomplete? As a much smaller file the bitmap could its self
> be very quickly generated in one pass when checksumming is enabled, with
> its starting state showing no pages having checksums.

Hmm... what if we took this a step further and actually stored the
checksums in a separate relation fork? That would make it pretty
simple to support enabling/disabling checksums for particular
relations. It would also allow us to have a wider checksum, like 32
or 64 bits rather than 16. I'm not scoffing at a 16-bit checksum,
because even that's enough to catch a very high percentage of errors,
but it wouldn't be terrible to be able to support a wider one, either.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Hmm... what if we took this a step further and actually stored the
> checksums in a separate relation fork? That would make it pretty
> simple to support enabling/disabling checksums for particular
> relations. It would also allow us to have a wider checksum, like 32
> or 64 bits rather than 16. I'm not scoffing at a 16-bit checksum,
> because even that's enough to catch a very high percentage of errors,
> but it wouldn't be terrible to be able to support a wider one, either.

What happens when you get an I/O failure on the checksum fork? Assuming
you're using 8K pages there, that would mean you can no longer verify
the integrity of between one and four thousand pages of data.

Not to mention the race condition problems associated with trying to be
sure the checksum updates hit the disk at the same time as the data-page
updates.

I think you really have to store the checksums *with* the data they're
supposedly protecting.

regards, tom lane

On Tue, Nov 13, 2012 at 4:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> What happens when you get an I/O failure on the checksum fork? Assuming
> you're using 8K pages there, that would mean you can no longer verify
> the integrity of between one and four thousand pages of data.

True... but you'll have succeeded in your central aim of determining
whether your hardware has crapped out. Answer: yes.

The existing code doesn't have any problem reporting back the user
those hardware failures which are reported to it by the OS. The only
reason for the feature is for the database to be able to detect
hardware failures in situations where the OS claims that everything is
working just fine.

> Not to mention the race condition problems associated with trying to be
> sure the checksum updates hit the disk at the same time as the data-page
> updates.
>
> I think you really have to store the checksums *with* the data they're
> supposedly protecting.

If torn pages didn't exist, I'd agree with you, but they do. Any
checksum feature is going to need to cope with the fact that, prior to
reaching consistency, there will be blocks on disk with checksums that
don't match, because 8kB writes are not atomic. We fix that by
unconditionally overwriting the possibly-torn pages with full-page
images, and we could simply update the checksum fork at the same time.
We don't have to do anything special to make sure that the next
checkpoint cycle successfully flushes both pages to disk before
declaring the checkpoint a success and moving the redo pointer; that
logic already exists.

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

On Mon, Nov 12, 2012 at 04:42:57PM -0800, Josh Berkus wrote:
> Jeff,
>
> > OK, so here's my proposal for a first patch (changes from Simon's
> > patch):
> >
> > * Add a flag to the postgres executable indicating that it should use
> > checksums on everything. This would only be valid if bootstrap mode is
> > also specified.
> > * Add a multi-state checksums flag in pg_control, that would have
> > three states: OFF, ENABLING, and ON. It would only be set to ON during
> > bootstrap, and in this first patch, it would not be possible to set
> > ENABLING.
> > * Remove GUC and use this checksums flag everywhere.
> > * Use the TLI field rather than the version field of the page header.
> > * Incorporate page number into checksum calculation (already done).
> >
> > Does this satisfy the requirements for a first step? Does it interfere
> > with potential future work?
>
> So the idea of this implementation is that checksums is something you
> set at initdb time, and if you want checksums on an existing database,
> it's a migration process (e.g. dump and reload)?
>
> I think that's valid as a first cut at this.

pg_upgrade will need to check for the checksum flag and throw an error
if it is present in the new cluster but not the old one.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 11/11/12 6:59 PM, Andrew Dunstan wrote:
> I haven't followed this too closely, but I did wonder several days ago
> why this wasn't being made an initdb-time decision.

One problem I see with this is that it would make regression testing
much more cumbersome. Basically, to do a proper job, you'd have to run
all the tests twice, once against each initdb setting. Either we
automate this, which would mean everyone's tests are now running almost
twice as long, or we don't, which would mean that some critical piece of
low-level code would likely not get wide testing.

Peter Eisentraut escribió:
> On 11/11/12 6:59 PM, Andrew Dunstan wrote:
> > I haven't followed this too closely, but I did wonder several days ago
> > why this wasn't being made an initdb-time decision.
>
> One problem I see with this is that it would make regression testing
> much more cumbersome. Basically, to do a proper job, you'd have to run
> all the tests twice, once against each initdb setting. Either we
> automate this, which would mean everyone's tests are now running almost
> twice as long, or we don't, which would mean that some critical piece of
> low-level code would likely not get wide testing.

We already have that problem with the isolation tests regarding
transaction isolation levels: the tests are only run with whatever is
the default_transaction_isolation setting, which is read committed in
all buildfarm installs; so repeatable read and serializable are only
tested when someone gets around to tweaking an installation manually. A
proposal has been floated to fix that, but it needs someone to actually
implement it.

I wonder if something similar could be used to handle this case as well.
I also wonder, though, if the existing test frameworks are really the
best mechanisms to verify block layer functionality.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/14/2012 02:01 PM, Alvaro Herrera wrote:
> Peter Eisentraut escribió:
>> On 11/11/12 6:59 PM, Andrew Dunstan wrote:
>>> I haven't followed this too closely, but I did wonder several days ago
>>> why this wasn't being made an initdb-time decision.
>> One problem I see with this is that it would make regression testing
>> much more cumbersome. Basically, to do a proper job, you'd have to run
>> all the tests twice, once against each initdb setting. Either we
>> automate this, which would mean everyone's tests are now running almost
>> twice as long, or we don't, which would mean that some critical piece of
>> low-level code would likely not get wide testing.
> We already have that problem with the isolation tests regarding
> transaction isolation levels: the tests are only run with whatever is
> the default_transaction_isolation setting, which is read committed in
> all buildfarm installs; so repeatable read and serializable are only
> tested when someone gets around to tweaking an installation manually. A
> proposal has been floated to fix that, but it needs someone to actually
> implement it.
>
> I wonder if something similar could be used to handle this case as well.
> I also wonder, though, if the existing test frameworks are really the
> best mechanisms to verify block layer functionality.

There is nothing to prevent a buildfarm owner from using different
settings - there is a stanza in the config file that provides for them
to do so in fact.

Maybe a saner thing to do though would be to run the isolation tests two
or three times with different PGOPTIONS settings. Maybe we need two ro
three targets in the isolation test Makefile for that.

Regarding checksums, I can add an option for the initdb that the
buildfarm script runs. We already run different tests for different
encodings. Of course, constant expanding like this won't scale, so we
need to pick the options we want to exrecise carefully.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Regarding checksums, I can add an option for the initdb that the
> buildfarm script runs. We already run different tests for different
> encodings. Of course, constant expanding like this won't scale, so we
> need to pick the options we want to exrecise carefully.

I thought the whole point of the buildfarm was to provide a scalable way
of exercising different combinations of options that individual
developers couldn't practically test. We might need a little more
coordination among buildfarm owners to ensure we get full coverage,
of course.

regards, tom lane

On 11/14/2012 03:06 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Regarding checksums, I can add an option for the initdb that the
>> buildfarm script runs. We already run different tests for different
>> encodings. Of course, constant expanding like this won't scale, so we
>> need to pick the options we want to exrecise carefully.
> I thought the whole point of the buildfarm was to provide a scalable way
> of exercising different combinations of options that individual
> developers couldn't practically test. We might need a little more
> coordination among buildfarm owners to ensure we get full coverage,
> of course.
>
>

Yes, true. So lets' wait and see how the checksums thing works out and
then we can tackle the buildfarm end. At any rate, I don't think the
buildfarm is a reason not to have this as an initdb setting.

cheers

andrew

> Hmm... what if we took this a step further and actually stored the
> checksums in a separate relation fork? That would make it pretty
> simple to support enabling/disabling checksums for particular
> relations. It would also allow us to have a wider checksum, like 32
> or 64 bits rather than 16. I'm not scoffing at a 16-bit checksum,
> because even that's enough to catch a very high percentage of errors,
> but it wouldn't be terrible to be able to support a wider one, either.

I don't remember exactly why this idea was sidelined before, but I don't
think there were any showstoppers. It does have some desirable
properties; most notably the ability to add checksums without a huge
effort, so perhaps the idea can be revived.

But there are some practical issues, as Tom points out. Another one is
that it's harder for external utilities (like pg_basebackup) to verify
checksums.

And I just had another thought: these pages of checksums would be data
pages, with an LSN. But as you clean ordinary data pages, you need to
constantly bump the LSN of the very same checksum page (because it
represents 1000 ordinary data pages); making it harder to actually clean
the checksum page and finish a checkpoint. Is this a practical concern
or am I borrowing trouble?

Regards,
Jeff Davis

On Tue, 2012-11-13 at 15:27 -0500, Robert Haas wrote:
> A small
> patch that gets committed is better than a big one that doesn't.

Here's a small patch (two, actually, because the TLI one is
uninteresting and noisy). It's based on Simon's patch, but with some
significant changes:

* I ripped out all of the handling for a mix of some checksummed and
some non-checksummed pages. No more control bits or page version stuff.
* I moved the checksum to the pd_tli field, and renamed it
pd_checksum.
* vm/fsm_extend were not setting the verification information for some
reason. I'm not sure why, but since it's now on/off for the entire
system, they need to do the same thing.
* Added a flag to pg_control called data_checksums. It is set by
initdb when the "-k"/"--data-checksums" option is specified (open for
discussion).
* Added a function in xlog.c that is a simple reader of the control
file flag.
* Got rid of page_checksums GUC.
* Incorporated the page number into the checksum calculation, to
detect pages that are transposed.

I'll do another pass to make sure I update all of the comments, and try
to self review it. So, slightly rough in some places.

Regards,
Jeff Davis

On Wed, Nov 14, 2012 at 6:24 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>> Hmm... what if we took this a step further and actually stored the
>> checksums in a separate relation fork? That would make it pretty
>> simple to support enabling/disabling checksums for particular
>> relations. It would also allow us to have a wider checksum, like 32
>> or 64 bits rather than 16. I'm not scoffing at a 16-bit checksum,
>> because even that's enough to catch a very high percentage of errors,
>> but it wouldn't be terrible to be able to support a wider one, either.
>
> I don't remember exactly why this idea was sidelined before, but I don't
> think there were any showstoppers. It does have some desirable
> properties; most notably the ability to add checksums without a huge
> effort, so perhaps the idea can be revived.
>
> But there are some practical issues, as Tom points out. Another one is
> that it's harder for external utilities (like pg_basebackup) to verify
> checksums.
>
> And I just had another thought: these pages of checksums would be data
> pages, with an LSN. But as you clean ordinary data pages, you need to
> constantly bump the LSN of the very same checksum page (because it
> represents 1000 ordinary data pages); making it harder to actually clean
> the checksum page and finish a checkpoint. Is this a practical concern
> or am I borrowing trouble?

Well, I think the invariant we'd need to maintain is as follows: every
page for which the checksum fork might be wrong must have an FPI
following the redo pointer. So, at the time we advance the redo
pointer, we need the checksum fork to be up-to-date for all pages for
which a WAL record was written after the old redo pointer except for
those for which a WAL record has again been written after the new redo
pointer. In other words, the checksum pages we write out don't need
to be completely accurate; the checksums for any blocks we know will
get clobbered anyway during replay don't really matter.

However, reading your comments, I do see one sticking point. If we
don't update the checksum page until a buffer is written out, which of
course makes a lot of sense, then during a checkpoint, we'd have to
flush all of the regular pages first and then all the checksum pages
afterward. Otherwise, the checksum pages wouldn't be sufficiently
up-to-date at the time we write them. There's no way to make that
happen just by fiddling with the LSN; rather, we'd need some kind of
two-pass algorithm over the buffer pool. That doesn't seem
unmanageable, but it's more complicated than what we do now.

I'm not sure we'd actually bother setting the LSN on the checksum
pages, because the action that prompts an update of a checksum page is
the decision to write out a non-checksum page, and that's not a
WAL-loggable action, so there's no obvious LSN to apply, and no
obvious need to apply one at all.

I'm also not quite sure what happens with full_page_writes=off. I
don't really see how to make this scheme work at all in that
environment. Keeping the checksum in the page seems to dodge quite a
few problems in that case ... as long as you assume that 8kB writes
really are atomic.

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

On Wed, 2012-11-14 at 21:22 -0500, Robert Haas wrote:
> > But there are some practical issues, as Tom points out. Another one is
> > that it's harder for external utilities (like pg_basebackup) to verify
> > checksums.

> Well, I think the invariant we'd need to maintain is as follows: every
> page for which the checksum fork might be wrong must have an FPI
> following the redo pointer. So, at the time we advance the redo
> pointer, we need the checksum fork to be up-to-date for all pages for
> which a WAL record was written after the old redo pointer except for
> those for which a WAL record has again been written after the new redo
> pointer. In other words, the checksum pages we write out don't need
> to be completely accurate; the checksums for any blocks we know will
> get clobbered anyway during replay don't really matter.

The issue about external utilities is a bigger problem than I realized
at first. Originally, I thought that it was just a matter of code to
associate the checksum with the data.

However, an external utility will never see a torn page while the system
is online (after recovery); but it *will* see an inconsistent view of
the checksum and the data if they are issued in separate write() calls.
So, the hazard of storing the checksum in a different place is not
equivalent to the existing hazard of a torn page.

Regards,
Jeff Davis

On Wed, 2012-11-14 at 17:40 -0800, Jeff Davis wrote:
> I'll do another pass to make sure I update all of the comments, and try
> to self review it.

Updated patches attached (the TLI patch wasn't changed though, only the
main checksums patch).

Changes:
* A lot of cleanup
* More testing
* Added check during pg_upgrade to make sure the checksum settings
match.
* Fixed output of pg_resetxlog to include information about checksums.
* fixed contrib/pageinspect, and included upgrade script for it
* removed code to skip the page hole during the checksum calculation.
We can reconsider if we think performance will be a real problem.
* I added the header bits back in, because we will need them when we
want to support enabling/disabling checksums when the system is online.

I also did quite a bit more testing, although it could use some
performance testing. I'll also probably do another review pass myself,
but I think it's in good shape.

Also, if performance of the checksum calculation itself turns out to be
a problem, we might consider modifying the algorithm to do multiple
bytes at a time.

One purpose of this patch is to establish the on-disk format for
checksums, so we shouldn't defer decisions that would affect that (e.g.
doing checksum calculation in larger chunks, ignoring the page hole, or
using a different scheme for the bits in the header).

Regards,
Jeff Davis

On Thu, Nov 15, 2012 at 2:44 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> The issue about external utilities is a bigger problem than I realized
> at first. Originally, I thought that it was just a matter of code to
> associate the checksum with the data.
>
> However, an external utility will never see a torn page while the system
> is online (after recovery); but it *will* see an inconsistent view of
> the checksum and the data if they are issued in separate write() calls.
> So, the hazard of storing the checksum in a different place is not
> equivalent to the existing hazard of a torn page.

I agree that the hazards are not equivalent, but I'm not sure I agree
that an external utility will never see a torn page while the system
is on-line. We have a bunch of code that essentially forces
full_page_writes=on during a base backup even if it's normally off. I
think that's necessary precisely because neither the 8kB write() nor
the unknown-sized-read used by the external copy program are
guaranteed to be atomic.

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

On Mon, 2012-11-19 at 11:48 -0500, Robert Haas wrote:
> I agree that the hazards are not equivalent, but I'm not sure I agree
> that an external utility will never see a torn page while the system
> is on-line. We have a bunch of code that essentially forces
> full_page_writes=on during a base backup even if it's normally off. I
> think that's necessary precisely because neither the 8kB write() nor
> the unknown-sized-read used by the external copy program are
> guaranteed to be atomic.

This seems like a standards question that we should be able to answer
definitively:

Is it possible for a reader to see a partial write if both use the same
block size?

Maybe the reason we need full page writes during base backup is because
we don't know the block size of the reader, but if we did know that it
was the same, it would be fine?

If that is not true, then I'm concerned about replicating corruption, or
backing up corrupt blocks over good ones. How do we prevent that? It
seems like a pretty major hole if we can't, because it means the only
safe replication is streaming replication; a base-backup is essentially
unsafe. And it means that even an online background checking utility
would be quite hard to do properly.

Regards,
Jeff Davis

On 2012-11-19 09:22:45 -0800, Jeff Davis wrote:
> On Mon, 2012-11-19 at 11:48 -0500, Robert Haas wrote:
> > I agree that the hazards are not equivalent, but I'm not sure I agree
> > that an external utility will never see a torn page while the system
> > is on-line. We have a bunch of code that essentially forces
> > full_page_writes=on during a base backup even if it's normally off. I
> > think that's necessary precisely because neither the 8kB write() nor
> > the unknown-sized-read used by the external copy program are
> > guaranteed to be atomic.
>
> This seems like a standards question that we should be able to answer
> definitively:
>
> Is it possible for a reader to see a partial write if both use the same
> block size?

Yes, definitely.

> If that is not true, then I'm concerned about replicating corruption, or
> backing up corrupt blocks over good ones. How do we prevent that? It
> seems like a pretty major hole if we can't, because it means the only
> safe replication is streaming replication; a base-backup is essentially
> unsafe. And it means that even an online background checking utility
> would be quite hard to do properly.

I am not sure I see the danger in the base backup case here? Why would
we have corrupted backup blocks? While postgres is running we won't see
such torn pages because its all done under proper locks...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 2012-11-19 at 18:30 +0100, Andres Freund wrote:
> Yes, definitely.

OK. I suppose that makes sense for large writes.

> > If that is not true, then I'm concerned about replicating corruption, or
> > backing up corrupt blocks over good ones. How do we prevent that? It
> > seems like a pretty major hole if we can't, because it means the only
> > safe replication is streaming replication; a base-backup is essentially
> > unsafe. And it means that even an online background checking utility
> > would be quite hard to do properly.
>
> I am not sure I see the danger in the base backup case here? Why would
> we have corrupted backup blocks? While postgres is running we won't see
> such torn pages because its all done under proper locks...

Yes, the blocks written *after* the checkpoint might have a bad checksum
that will be fixed during recovery. But the blocks written *before* the
checkpoint should have a valid checksum, but if they don't, then
recovery doesn't know about them.

So, we can't verify the checksums in the base backup because it's
expected that some blocks will fail the check, and they can be fixed
during recovery. That gives us no protection for blocks that were truly
corrupted and written long before the last checkpoint.

I suppose if we could somehow differentiate the blocks, that might work.
Maybe look at the LSN and only validate blocks written before the
checkpoint? But of course, that's a problem because a corrupt block
might have the wrong LSN (in fact, it's likely, because garbage is more
likely to make the LSN too high than too low).

Regards,
Jeff Davis

On Mon, 2012-11-19 at 10:35 -0800, Jeff Davis wrote:
> Yes, the blocks written *after* the checkpoint might have a bad checksum
> that will be fixed during recovery. But the blocks written *before* the
> checkpoint should have a valid checksum, but if they don't, then
> recovery doesn't know about them.
>
> So, we can't verify the checksums in the base backup because it's
> expected that some blocks will fail the check, and they can be fixed
> during recovery. That gives us no protection for blocks that were truly
> corrupted and written long before the last checkpoint.
>
> I suppose if we could somehow differentiate the blocks, that might work.
> Maybe look at the LSN and only validate blocks written before the
> checkpoint? But of course, that's a problem because a corrupt block
> might have the wrong LSN (in fact, it's likely, because garbage is more
> likely to make the LSN too high than too low).

It might be good enough here to simply retry the checksum verification
if it fails for any block. Postgres shouldn't be issuing write()s for
the same block very frequently, and they shouldn't take very long, so
the chances of failing several times seems vanishingly small unless it's
a real failure.

Through a suitably complex mechanism, I think we can be more sure. The
external program could wait for a checkpoint (or force one manually),
and then recalculate the checksum for that page. If checksum is the same
as the last time, then we know the block is bad (because the checkpoint
would have waited for any writes in progress). If the checksum does
change, then we assume postgres must have modified it since the backup
started, so we can assume that we have a full page image to fix it. (A
checkpoint is a blunt tool here, because all we need to do is wait for
the write() call to finish, but it suffices.)

That complexity is probably not required, and simply retrying a few
times is probably much more practical. But it still bothers me a little
to think that the external tool could falsely indicate a checksum
failure, however remote that chance.

Regards,
Jeff Davis

Updated both patches.

Changes:
* Moved the changes to pageinspect into the TLI patch, because it
makes more sense to be a part of that patch and it also reduces the size
of the main checksums patch.
* Fix off-by-one bug in checksum calculation
* Replace "VerificationInfo" in the function names with "Checksum",
which is shorter.
* Make the checksum algorithm process 4 bytes at a time and sum into a
signed 64-bit int, which is faster than byte-at-a-time. Also, forbid
zero in either byte of the checksum, because that seems like a good
idea.

I've done quite a bit of testing at this point, and everything seems
fine to me. I've tested various kinds of errors (bytes being modified or
zeroed at various places of the header and data areas, transposed pages)
at 8192 and 32768 page sizes. I also looked at the distribution of
checksums in various ways (group by checksum % <prime> for various
primes, and not seeing any skew), and I didn't see any worrying
patterns.

Regards,
Jeff Davis

On 26 November 2012 02:32, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Updated both patches.
>
> Changes:
> * Moved the changes to pageinspect into the TLI patch, because it
> makes more sense to be a part of that patch and it also reduces the size
> of the main checksums patch.
> * Fix off-by-one bug in checksum calculation
> * Replace "VerificationInfo" in the function names with "Checksum",
> which is shorter.
> * Make the checksum algorithm process 4 bytes at a time and sum into a
> signed 64-bit int, which is faster than byte-at-a-time. Also, forbid
> zero in either byte of the checksum, because that seems like a good
> idea.
>
> I've done quite a bit of testing at this point, and everything seems
> fine to me. I've tested various kinds of errors (bytes being modified or
> zeroed at various places of the header and data areas, transposed pages)
> at 8192 and 32768 page sizes. I also looked at the distribution of
> checksums in various ways (group by checksum % <prime> for various
> primes, and not seeing any skew), and I didn't see any worrying
> patterns.

I think the way forwards for this is...

1. Break out the changes around inCommit flag, since that is just
uncontroversial refactoring. I can do that. That reduces the noise
level in the patch and makes it easier to understand the meaningful
changes.

2. Produce an SGML docs page that describes how this works, what the
limitations and tradeoffs are. "Reliability & the WAL" could use an
extra section2 header called Checksums (wal.sgml). This is essential
for users AND reviewers to ensure everybody has understood this (heck,
I can't remember everything about this either...)

3. I think we need an explicit test of this feature (as you describe
above), rather than manual testing. corruptiontester?

4. We need some general performance testing to show whether this is
insane or not.

But this looks in good shape for commit otherwise.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 3 December 2012 09:56, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> I think the way forwards for this is...
>
> 1. Break out the changes around inCommit flag, since that is just
> uncontroversial refactoring. I can do that. That reduces the noise
> level in the patch and makes it easier to understand the meaningful
> changes.

Done.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 26 November 2012 02:32, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> * Make the checksum algorithm process 4 bytes at a time and sum into a
> signed 64-bit int, which is faster than byte-at-a-time. Also, forbid
> zero in either byte of the checksum, because that seems like a good
> idea.

Like that, especially the bit where we use the blocknumber as the seed
for the checksum, so it will detect transposed pages. That's also a
really neat way of encrypting the data for anybody that tries to
access things via direct anonymous file access.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 2012-12-03 at 13:16 +0000, Simon Riggs wrote:
> On 3 December 2012 09:56, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> > I think the way forwards for this is...
> >
> > 1. Break out the changes around inCommit flag, since that is just
> > uncontroversial refactoring. I can do that. That reduces the noise
> > level in the patch and makes it easier to understand the meaningful
> > changes.
>
> Done.

Thank you.

One minor thing I noticed: it looks like nwaits is a useless variable.
Your original checksums patch used it to generate a warning, but now
that is gone. It's not throwing a compiler warning for some reason.

Regards,
Jeff Davis

On Mon, 2012-12-03 at 09:56 +0000, Simon Riggs wrote:
> 1. Break out the changes around inCommit flag, since that is just
> uncontroversial refactoring. I can do that. That reduces the noise
> level in the patch and makes it easier to understand the meaningful
> changes.

Done by you.

> 2. Produce an SGML docs page that describes how this works, what the
> limitations and tradeoffs are. "Reliability & the WAL" could use an
> extra section2 header called Checksums (wal.sgml). This is essential
> for users AND reviewers to ensure everybody has understood this (heck,
> I can't remember everything about this either...)

Agreed. It looks like it would fit best under the Reliability section,
because it's not directly related to WAL. I'll write something up.

> 3. I think we need an explicit test of this feature (as you describe
> above), rather than manual testing. corruptiontester?

I agree, but I'm not 100% sure how to proceed. I'll look at Kevin's
tests for SSI and see if I can do something similar, but suggestions are
welcome. A few days away, at the earliest.

> 4. We need some general performance testing to show whether this is
> insane or not.

My understanding is that Greg Smith is already working on tests here, so
I will wait for his results.

> But this looks in good shape for commit otherwise.

Great!

For now, I rebased the patches against master, and did some very minor
cleanup.

Regards,
Jeff Davis

On Tue, 2012-12-04 at 01:03 -0800, Jeff Davis wrote:
> > 3. I think we need an explicit test of this feature (as you describe
> > above), rather than manual testing. corruptiontester?
>
> I agree, but I'm not 100% sure how to proceed. I'll look at Kevin's
> tests for SSI and see if I can do something similar, but suggestions are
> welcome. A few days away, at the earliest.

I looked into this. The SSI tests still use pg_regress to start/stop the
server, and make use of a lot more of the pg_regress framework.
pg_regress doesn't fit what I need to do, at all.

For me, each test involves a fresh initdb, followed by a small data load
(at least a few pages). Then, I shut down the server, inject the faults
under test, and start the server back up. Then, I count the table and
expect an error. Then I throw away the data directory. (I can shortcut
some of the initdb and load time by keeping a good copy of the table
throughout the whole set of tests and copying it back, but that's just a
detail.)

So, I could try to write a test framework in C that would be a candidate
to include with the main distribution and be run by the buildfarm, but
that would be a lot of work. Even then, I couldn't easily abstract away
these kinds of tests into text files, unless I invent a language that is
suitable for describing disk faults to inject.

Or, I could write up a test framework in ruby or python, using the
appropriate pg driver, and some not-so-portable shell commands to start
and stop the server. Then, I can publish that on this list, and that
would at least make it easier to test semi-manually and give greater
confidence in pre-commit revisions.

Suggestions?

Regards,
Jeff Davis

On Tue, Dec 4, 2012 at 6:17 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Or, I could write up a test framework in ruby or python, using the
> appropriate pg driver, and some not-so-portable shell commands to start
> and stop the server. Then, I can publish that on this list, and that
> would at least make it easier to test semi-manually and give greater
> confidence in pre-commit revisions.

That latter approach is similar to what happened with SSI's isolation
tester. It started out in Python, and then Heikki rewrote it in C.
If Python/Ruby code is massively simpler to write than the C code,
that might be a good way to start out. It'll be an aid to reviewers
even if neither it nor any descendent gets committed.

Frankly, I think some automated testing harness (written in C or Perl)
that could do fault-injection tests as part of the buildfarm would be
amazingly awesome. I'm drooling just thinking about it. But I guess
that's getting ahead of myself.

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

On 5 December 2012 23:40, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 4, 2012 at 6:17 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>> Or, I could write up a test framework in ruby or python, using the
>> appropriate pg driver, and some not-so-portable shell commands to start
>> and stop the server. Then, I can publish that on this list, and that
>> would at least make it easier to test semi-manually and give greater
>> confidence in pre-commit revisions.
>
> That latter approach is similar to what happened with SSI's isolation
> tester. It started out in Python, and then Heikki rewrote it in C.
> If Python/Ruby code is massively simpler to write than the C code,
> that might be a good way to start out. It'll be an aid to reviewers
> even if neither it nor any descendent gets committed.
>
> Frankly, I think some automated testing harness (written in C or Perl)
> that could do fault-injection tests as part of the buildfarm would be
> amazingly awesome. I'm drooling just thinking about it. But I guess
> that's getting ahead of myself.

Agreed, though we can restrict that to a few things at first.

* Zeroing pages, making pages all 1s
* Transposing pages
* Moving chunks of data sideways in a block
* Flipping bits randomly
* Flipping data endianness
* Destroying particular catalog tables or structures

etc

As a contrib module, so we can be sure to never install it. ;-)

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/5/12 6:49 PM, Simon Riggs wrote:
> * Zeroing pages, making pages all 1s
> * Transposing pages
> * Moving chunks of data sideways in a block
> * Flipping bits randomly
> * Flipping data endianness
> * Destroying particular catalog tables or structures

I can take this on, as part of the QA around checksums working as
expected. The result would be a Python program; I don't have quite
enough time to write this in C or re-learn Perl to do it right now. But
this won't be a lot of code. If it's tossed one day as simply a
prototype for something more permanent, I think it's still worth doing now.

The UI I'm thinking of for what I'm going to call pg_corrupt is a CLI
that asks for:

-A relation name
-Corruption type (an entry from this list)
-How many blocks to touch

I'll just loop based on the count, randomly selecting a block each time
and messing with it in that way.

The randomness seed should be printed as part of the output, so that
it's possible re-create the damage exactly later. If the server doesn't
handle it correctly, we'll want to be able to replicate the condition it
choked on exactly later, just based on the tool's log output.

Any other requests?

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Wed, 2012-12-12 at 17:52 -0500, Greg Smith wrote:
> I can take this on, as part of the QA around checksums working as
> expected. The result would be a Python program; I don't have quite
> enough time to write this in C or re-learn Perl to do it right now. But
> this won't be a lot of code. If it's tossed one day as simply a
> prototype for something more permanent, I think it's still worth doing now.
>
> The UI I'm thinking of for what I'm going to call pg_corrupt is a CLI
> that asks for:
>
> -A relation name
> -Corruption type (an entry from this list)
> -How many blocks to touch
>
> I'll just loop based on the count, randomly selecting a block each time
> and messing with it in that way.
>
> The randomness seed should be printed as part of the output, so that
> it's possible re-create the damage exactly later. If the server doesn't
> handle it correctly, we'll want to be able to replicate the condition it
> choked on exactly later, just based on the tool's log output.
>
> Any other requests?

After some thought, I don't see much value in introducing multiple
instances of corruption at a time. I would think that the smallest unit
of corruption would be the hardest to detect, so by introducing many of
them in one pass makes it easier to detect.

For example, if we introduce an all-ones page, and also transpose two
pages, the all-ones error might be detected even if the transpose error
is not being detected properly. And we'd not know that the transpose
error was not being detected, because the error appears as soon as it
sees the all-ones page.

Does it make sense to have a separate executable (pg_corrupt) just for
corrupting the data as a test? Or should it be part of a
corruption-testing harness (pg_corruptiontester?), that introduces the
corruption and then verifies that it's properly detected?

Regards,
Jeff Davis

On 12/14/12 3:00 PM, Jeff Davis wrote:
> After some thought, I don't see much value in introducing multiple
> instances of corruption at a time. I would think that the smallest unit
> of corruption would be the hardest to detect, so by introducing many of
> them in one pass makes it easier to detect.

That seems reasonable. It would eliminate a lot of issues with
reproducing a fault too. I can just print the impacted block number
presuming it will show up in a log, and make it possible to override
picking one at random with a command line input.

> Does it make sense to have a separate executable (pg_corrupt) just for
> corrupting the data as a test? Or should it be part of a
> corruption-testing harness (pg_corruptiontester?), that introduces the
> corruption and then verifies that it's properly detected?

Let me see what falls out of the coding, I don't think this part needs
to get nailed down yet. Building a corruption testing harness is going
to involve a lot of creating new clusters and test data to torture.
It's a different style of problem than injecting faults in the first place.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
>> -A relation name
>> -Corruption type (an entry from this list)
>> -How many blocks to touch
>>
>> I'll just loop based on the count, randomly selecting a block each time
>> and messing with it in that way.

For the messing with it part, did you consider zzuf?

http://caca.zoy.org/wiki/zzuf

> Does it make sense to have a separate executable (pg_corrupt) just for
> corrupting the data as a test? Or should it be part of a
> corruption-testing harness (pg_corruptiontester?), that introduces the
> corruption and then verifies that it's properly detected?

Maybe we need our own zzuf implementation, though.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 14 December 2012 20:15, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> On 12/14/12 3:00 PM, Jeff Davis wrote:
>>
>> After some thought, I don't see much value in introducing multiple
>> instances of corruption at a time. I would think that the smallest unit
>> of corruption would be the hardest to detect, so by introducing many of
>> them in one pass makes it easier to detect.
>
>
> That seems reasonable. It would eliminate a lot of issues with reproducing
> a fault too. I can just print the impacted block number presuming it will
> show up in a log, and make it possible to override picking one at random
> with a command line input.

Discussing this makes me realise that we need a more useful response
than just "your data is corrupt", so user can respond "yes, I know,
I'm trying to save whats left".

We'll need a way of expressing some form of corruption tolerance.
zero_damaged_pages is just insane, much better if we set
corruption_tolerance = N to allow us to skip N corrupt pages before
failing, with -1 meaning keep skipping for ever. Settable by superuser
only.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> Discussing this makes me realise that we need a more useful response
> than just "your data is corrupt", so user can respond "yes, I know,
> I'm trying to save whats left".

> We'll need a way of expressing some form of corruption tolerance.
> zero_damaged_pages is just insane, much better if we set
> corruption_tolerance = N to allow us to skip N corrupt pages before
> failing, with -1 meaning keep skipping for ever. Settable by superuser
> only.

Define "skip". Extra points if it makes sense for an index. And what
about things like pg_clog pages?

regards, tom lane

On 17 December 2012 19:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> Discussing this makes me realise that we need a more useful response
>> than just "your data is corrupt", so user can respond "yes, I know,
>> I'm trying to save whats left".
>
>> We'll need a way of expressing some form of corruption tolerance.
>> zero_damaged_pages is just insane, much better if we set
>> corruption_tolerance = N to allow us to skip N corrupt pages before
>> failing, with -1 meaning keep skipping for ever. Settable by superuser
>> only.
>
> Define "skip".

Allow data access, but accept that the answer is silently incomplete.
Not really much difference from zero_damaged_pages which just removes
the error by removing any chance of repair or recovery, and then
silently gives the wrong answer.

> Extra points if it makes sense for an index.

I guess not, but that's no barrier to it working on heap pages only,
in my suggested use case.

> And what about things like pg_clog pages?

SLRUs aren't checksummed because of their lack of header space.
Perhaps that is a major point against the patch.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 2012-12-17 at 19:14 +0000, Simon Riggs wrote:
> We'll need a way of expressing some form of corruption tolerance.
> zero_damaged_pages is just insane,

The main problem I see with zero_damaged_pages is that it could
potentially write out the zero page, thereby really losing your data if
it wasn't already lost. (Of course, we document that you should have a
backup first, but it's still dangerous). I assume that this is the same
problem you are talking about.

I suppose we could have a new ReadBufferMaybe function that would only
be used by a sequential scan; and then just skip over the page if it's
corrupt, depending on a GUC. That would at least allow sequential scans
to (partially) work, which might be good enough for some data recovery
situations. If a catalog index is corrupted, that could just be rebuilt.
Haven't thought about the details, though.

Regards,
Jeff Davis

On 18 December 2012 02:21, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Mon, 2012-12-17 at 19:14 +0000, Simon Riggs wrote:
>> We'll need a way of expressing some form of corruption tolerance.
>> zero_damaged_pages is just insane,
>
> The main problem I see with zero_damaged_pages is that it could
> potentially write out the zero page, thereby really losing your data if
> it wasn't already lost. (Of course, we document that you should have a
> backup first, but it's still dangerous). I assume that this is the same
> problem you are talking about.

I think we should discuss whether we accept my premise? Checksums will
actually detect more errors than we see now, and people will want to
do something about that. Returning to backup is one way of handling
it, but on a busy production system with pressure on, there is
incentive to implement a workaround, not a fix. It's not an easy call
to say "we've got 3 corrupt blocks, so I'm going to take the whole
system offline while I restore from backup".

If you do restore from backup, and the backup also contains the 3
corrupt blocks, what then?

Clearly part of the response could involve pg_dump on the damaged
structure, at some point.

> I suppose we could have a new ReadBufferMaybe function that would only
> be used by a sequential scan; and then just skip over the page if it's
> corrupt, depending on a GUC. That would at least allow sequential scans
> to (partially) work, which might be good enough for some data recovery
> situations. If a catalog index is corrupted, that could just be rebuilt.
> Haven't thought about the details, though.

Not sure if you're being facetious here or not. Mild reworking of the
logic for heap page access could cope with a NULL buffer response and
subsequent looping, which would allow us to run pg_dump against a
damaged table to allow data to be saved, keeping file intact for
further analysis.

I'm suggesting we work a little harder than "your block is corrupt"
and give some thought to what the user will do next. Indexes are a
good case, because we can/should report the block error, mark the
index as invalid and then hint that it should be rebuilt.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/18/12 3:17 AM, Simon Riggs wrote:
> Clearly part of the response could involve pg_dump on the damaged
> structure, at some point.

This is the main thing I wanted to try out more, once I have a decent
corruption generation tool. If you've corrupted a single record but can
still pg_dump the remainder, that seems the best we can do to help
people recover from that. Providing some documentation on how to figure
out what rows are in that block, presumably by using the contrib
inspection tools, would be helpful too.

> Indexes are a good case, because we can/should report the block error, mark the
> index as invalid and then hint that it should be rebuilt.

Marking a whole index invalid because there's one bad entry has enough
downsides that I'm not sure how much we'd want to automate that. Not
having that index available could easily result in an effectively down
system due to low performance. The choices are uglier if it's backing a
unique constraint.

In general, what I hope people will be able to do is switch over to
their standby server, and then investigate further. I think it's
unlikely that people willing to pay for block checksums will only have
one server. Having some way to nail down if the same block is bad on a
given standby seems like a useful interface we should offer, and it
shouldn't take too much work. Ideally you won't find the same
corruption there. I'd like a way to check the entirety of a standby for
checksum issues, ideally run right after it becomes current. It seems
the most likely way to see corruption on one of those is to replicate a
corrupt block.

There is no good way to make the poor soul who has no standby server
happy here. You're just choosing between bad alternatives. The first
block error is often just that--the first one, to be joined by others
soon afterward. My experience at how drives fail says the second error
is a lot more likely after you've seen one.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Tue, 2012-12-18 at 08:17 +0000, Simon Riggs wrote:
> I think we should discuss whether we accept my premise? Checksums will
> actually detect more errors than we see now, and people will want to
> do something about that. Returning to backup is one way of handling
> it, but on a busy production system with pressure on, there is
> incentive to implement a workaround, not a fix. It's not an easy call
> to say "we've got 3 corrupt blocks, so I'm going to take the whole
> system offline while I restore from backup".

Up until now, my assumption has generally been that, upon finding the
corruption, the primary course of action is taking that server down
(hopefully you have a good replica), and do some kind of restore or sync
a new replica.

It sounds like you are exploring other possibilities.

> > I suppose we could have a new ReadBufferMaybe function that would only
> > be used by a sequential scan; and then just skip over the page if it's
> > corrupt, depending on a GUC. That would at least allow sequential scans
> > to (partially) work, which might be good enough for some data recovery
> > situations. If a catalog index is corrupted, that could just be rebuilt.
> > Haven't thought about the details, though.
>
> Not sure if you're being facetious here or not.

No. It was an incomplete thought (as I said), but sincere.

> Mild reworking of the
> logic for heap page access could cope with a NULL buffer response and
> subsequent looping, which would allow us to run pg_dump against a
> damaged table to allow data to be saved, keeping file intact for
> further analysis.

Right.

> I'm suggesting we work a little harder than "your block is corrupt"
> and give some thought to what the user will do next. Indexes are a
> good case, because we can/should report the block error, mark the
> index as invalid and then hint that it should be rebuilt.

Agreed; this applies to any derived data.

I don't think it will be very practical to keep a server running in this
state forever, but it might give enough time to reach a suitable
maintenance window.

Regards,
Jeff Davis

On Tue, 2012-12-18 at 04:06 -0500, Greg Smith wrote:
> Having some way to nail down if the same block is bad on a
> given standby seems like a useful interface we should offer, and it
> shouldn't take too much work. Ideally you won't find the same
> corruption there. I'd like a way to check the entirety of a standby for
> checksum issues, ideally run right after it becomes current. It seems
> the most likely way to see corruption on one of those is to replicate a
> corrupt block.

Part of the design is that pg_basebackup would verify checksums during
replication, so we should not replicate corrupt blocks (of course,
that's not implemented yet, so it's still a concern for now).

And we can also have ways to do background/offline checksum verification
with a separate utility.

Regards,
Jeff Davis

On Tue, 2012-12-04 at 01:03 -0800, Jeff Davis wrote:
> > 4. We need some general performance testing to show whether this is
> > insane or not.

I ran a few tests.

Test 1 - find worst-case overhead for the checksum calculation on write:

fsync = off
bgwriter_lru_maxpages = 0
shared_buffers = 1024MB
checkpoint_segments = 64
autovacuum = off

The idea is to prevent interference from the bgwriter or autovacuum.
Also, I turn of fsync so that it's measuring the calculation overhead,
not the effort of actually writing to disk.

drop table foo;
create table foo(i int, j int) with (fillfactor=50);
create index foo_idx on foo(i);
insert into foo select g%25, -1 from generate_series(1,10000000) g;
checkpoint;
-- during the following sleep, issue an OS "sync"
-- to make test results more consistent
select pg_sleep(30);
\timing on
update foo set j=-1 where i = 0;
select pg_sleep(2);
checkpoint;
update foo set j=-1 where i = 0;
select pg_sleep(2);
checkpoint;
update foo set j=-1 where i = 0;
select pg_sleep(2);
checkpoint;
\timing off

I am measuring the time of the CHECKPOINT command, not the update. The
update is just to dirty all of the pages (they should all be HOT
updates). Without checksums, it takes about 400ms. With checksums, it
takes about 500ms. That overhead is quite low, considering that the
bottleneck is almost always somewhere else (like actually writing to
disk).

Test 2 - worst-case overhead for calculating checksum while reading data

Same configuration as above. This time, just load a big table:

drop table foo;
create table foo(i int, j int) with (fillfactor=50);
insert into foo select g%25, -1 from generate_series(1,10000000) g;
-- make sure hint bits and PD_ALL_VISIBLE are set everywhere
select count(*) from foo;
vacuum;
vacuum;
vacuum;
select relfilenode from pg_class where relname='foo';

Then shut down the server and restart it. Then do a "cat
data/base/12055/XXXX* > /dev/null" to get the table loaded into the OS
buffer cache. Then do:

\timing on
SELECT COUNT(*) FROM foo;

So, shared buffers are cold, but OS cache is warm. This should test the
overhead of going from the OS to shared buffers, which requires the
checksum calculation. Without checksums is around 820ms; with checksums
around 970ms. Again, this is quite reasonable, because I would expect
the bottleneck to be reading from the disk rather than the calculation
itself.

Test 3 - worst-case WAL overhead

For this test, I also left fsync off, because I didn't want to test the
effort to flush WAL (which shouldn't really be required for this test,
anyway). This was simpler:

drop table foo;
create table foo(i int, j int) with (fillfactor=50);
insert into foo select g%25, -1 from generate_series(1,10000000) g;
checkpoint;
select pg_sleep(1);
checkpoint;
select pg_sleep(30); -- do an OS "sync" while this is running
\timing on
SELECT COUNT(*) FROM foo;

Without checksums, it takes about 1000ms. With checksums, about 2350ms.
I also tested with checksums but without the CHECKPOINT commands above,
and it was also 1000ms.

This test is more plausible than the other two, so it's more likely to
be a real problem. So, the biggest cost of checksums is, by far, the
extra full-page images in WAL, which matches our expectations.

Regards,
Jeff Davis

On Tue, Dec 18, 2012 at 04:06:02AM -0500, Greg Smith wrote:
> On 12/18/12 3:17 AM, Simon Riggs wrote:
> >Clearly part of the response could involve pg_dump on the damaged
> >structure, at some point.
>
> This is the main thing I wanted to try out more, once I have a
> decent corruption generation tool. If you've corrupted a single
> record but can still pg_dump the remainder, that seems the best we
> can do to help people recover from that. Providing some
> documentation on how to figure out what rows are in that block,
> presumably by using the contrib inspection tools, would be helpful
> too.

FWIW, Postgres is pretty resiliant against corruption. I've maintained
a postgres db on a server with bad memory (don't ask) and since most
scrambling was in text strings you just got funny output sometimes. The
most common failure was a memory allocation failure as postgres tried
to copy a datum whose length field was correupted.

If things went really wonky you could identify the bad tuples by hand
and then delete them by ctid. Regular reindexing helped too.

All I'm saying is that a mode where you log a warning but proceed
anyway is useful. It won't pin down the exact error, but it will tell
you where to look and help find the non-obvious corruption (so you can
possibly fix it by hand).

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

On Tue, 2012-12-04 at 01:03 -0800, Jeff Davis wrote:
> For now, I rebased the patches against master, and did some very minor
> cleanup.

I think there is a problem here when setting PD_ALL_VISIBLE. I thought I
had analyzed that before, but upon review, it doesn't look right.
Setting PD_ALL_VISIBLE needs to be associated with a WAL action somehow,
and a bumping of the LSN, otherwise there is a torn page hazard.

The solution doesn't seem particularly difficult, but there are a few
strange aspects and I'm not sure exactly which path I should take.

First of all, the relationship between MarkBufferDirty and
SetBufferCommitInfoNeedsSave is a little confusing. The comment over
MarkBufferDirty is confusing because it says that the caller must have
an exclusive lock, or else bad data could be written. But that doesn't
have to do with marking the buffer dirty, that has to do with the data
page change you make while you are marking it dirty -- if it's a single
bit change, then there is no risk that I can see.

In the current code, the only real code difference between the two is
that SetBufferCommitInfoNeedsSave might fail to mark the buffer dirty if
there is a race. So, in the current code, we could actually combine the
two by passing a "force" flag (if true, behaves like MarkBufferDirty, if
false, behaves like SetBufferCommitInfoNeedsSave).

The checksums patch also introduces another behavior into
SetBufferCommitInfoNeedsSave, which is to write an XLOG_HINT WAL record
if checksums are enabled (to avoid torn page hazards). That's only
necessary for changes where the caller does not write WAL itself and
doesn't bump the LSN of the data page. (There's a reason the caller
can't easily write the XLOG_HINT WAL itself.) So, we could introduce
another flag "needsWAL" that would control whether we write the
XLOG_HINT WAL or not (only applies with checksums on, of course).

The reason for all of this is because the setting of PD_ALL_VISIBLE does
not fit MarkBufferDirty, because MarkBufferDirty does not write the
XLOG_HINT WAL and neither does the caller. But it also doesn't fit
SetBufferCommitInfoNeedsSave, because that is subject to a race. If
MarkBufferDirty had the signature:

MarkBufferDirty(Buffer buffer, bool force, bool needsWAL)

then "normal" page changes would look like:
MarkBufferDirty(buffer, true, false)
setting PD_ALL_VISIBLE would look like:
MarkBufferDirty(buffer, true, true)
and setting a hint would look like:
MarkBufferDirty(buffer, false, true)

Another approach would be for the caller who sets PD_ALL_VISIBLE to
write WAL. But that requires inventing a new WAL record or chaining the
heap block onto the wal entry when doing visibilitymap_set (only
necessary when checksums are on). That seems somewhat of a hack, but
perhaps it's not too bad.

Also, I have another patch posted that is removing PD_ALL_VISIBLE
entirely, which is dampening my enthusiasm to do too much work that
might be thrown away. So right now, I'm leaning toward just adding the
heap buffer to the WAL chain during visibilitymap_set.

Thoughts?

Regards,
Jeff Davis

On 10 January 2013 06:06, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> The checksums patch also introduces another behavior into
> SetBufferCommitInfoNeedsSave, which is to write an XLOG_HINT WAL record
> if checksums are enabled (to avoid torn page hazards). That's only
> necessary for changes where the caller does not write WAL itself and
> doesn't bump the LSN of the data page. (There's a reason the caller
> can't easily write the XLOG_HINT WAL itself.) So, we could introduce
> another flag "needsWAL" that would control whether we write the
> XLOG_HINT WAL or not (only applies with checksums on, of course).

That wouldn't work because it can't know the exact answer to that, but
the way the patch does this is already correct.

XLOG_HINT_WAL doesn't always write a WAL record, it only does it when
necessary. See XLogInsert()

Didn't fully understand other comments. Do we we need an answer now?
My head is somewhere else.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> > The checksums patch also introduces another behavior into
> > SetBufferCommitInfoNeedsSave, which is to write an XLOG_HINT WAL record
> > if checksums are enabled (to avoid torn page hazards). That's only
> > necessary for changes where the caller does not write WAL itself and
> > doesn't bump the LSN of the data page. (There's a reason the caller
> > can't easily write the XLOG_HINT WAL itself.) So, we could introduce
> > another flag "needsWAL" that would control whether we write the
> > XLOG_HINT WAL or not (only applies with checksums on, of course).
>
> That wouldn't work because it can't know the exact answer to that, but
> the way the patch does this is already correct.

The name I chose was poor, but the flag should mean "the caller does not
write WAL associated with this action". If that flag is true, and if
checksums are enabled, then it would do an XLogInsert, which may write
WAL (depending on the LSN check).

That part of the patch is correct currently, but the problem is with
updates to PD_ALL_VISIBLE. Let me try to explain again:

Calls to PageSetAllVisible are not directly associated with a WAL
action, but they are associated with a call to MarkBufferDirty and do
have an exclusive content lock on the buffer. There's a torn page hazard
there for checksums, because without any WAL action associated with the
data page, there is no backup page.

One idea might be to use SetBufferCommitInfoNeedsSave (which will write
WAL if necessary) instead of MarkBufferDirty. But that is unsafe,
because it might not actually mark the buffer dirty due to a race
(documented in SetBufferCommitInfoNeedsSave). So that's why I wanted to
refactor MarkBufferDirty/SetBufferCommitInfoNeedsSave, to separate the
concept that it may need a WAL record from the concept that actually
dirtying the page is optional.

Another idea is to make the WAL action for visibilitymap_set have
another item in the chain pointing to the heap buffer, and bump the heap
LSN.

Regards,
Jeff Davis

On 12/19/12 6:30 PM, Jeff Davis wrote:
> The idea is to prevent interference from the bgwriter or autovacuum.
> Also, I turn of fsync so that it's measuring the calculation overhead,
> not the effort of actually writing to disk.

With my test server issues sorted, what I did was setup a single 7200RPM
drive with a battery-backed write cache card. That way fsync doesn't
bottleneck things. And I to realized that limit had to be cracked
before anything use useful could be done. Having the BBWC card is a bit
better than fsync=off, because we'll get something more like the
production workload out of it. I/O will be realistic, but limited to
only one one drive can pull off.

> Without checksums, it takes about 1000ms. With checksums, about 2350ms.
> I also tested with checksums but without the CHECKPOINT commands above,
> and it was also 1000ms.

I think we need to use lower checkpoint_segments to try and trigger more
checkpoints. My 10 minute pgbench-tool runs will normally have at most
3 checkpoints. I would think something like 10 would be more useful, to
make sure we're spending enough time seeing extra WAL writes;

> This test is more plausible than the other two, so it's more likely to
> be a real problem. So, the biggest cost of checksums is, by far, the
> extra full-page images in WAL, which matches our expectations.

What I've done with pgbench-tools is actually measure the amount of WAL
from the start to the end of the test run. To analyze it you need to
scale it a bit; computing "wal bytes / commit" seems to work.

pgbench-tools also launches vmstat and isstat in a way that it's
possible to graph the values later. The interesting results I'm seeing
are when the disk is about 80% busy and when it's 100% busy.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

First rev of a simple corruption program is attached, in very C-ish
Python. The parameters I settled on are to accept a relation name, byte
offset, byte value, and what sort of operation to do: overwrite, AND,
OR, XOR. I like XOR here because you can fix it just by running the
program again. Rewriting this in C would not be terribly difficult, and
most of the time spent on this version was figuring out what to do.

This follows Jeff's idea that the most subtle corruption is the hardest
to spot, so testing should aim at the smallest unit of change. If you
can spot a one bit error in an unused byte of a page, presumably that
will catch large errors like a byte swap. I find some grim amusement
that the checksum performance testing I've been trying to do got stuck
behind a problem with a tiny, hard to detect single bit of corruption.

Here's pgbench_accounts being corrupted, the next to last byte on this line:

$ pgbench -i -s 1
$ ./pg_corrupt pgbench_accounts show
Reading byte 0 within file /usr/local/var/postgres/base/16384/25242
Current byte= 0 / $00
$ hexdump /usr/local/var/postgres/base/16384/25242 | head
0000000 00 00 00 00 00 00 00 00 00 00 04 00 0c 01 80 01
...
$ ./pg_corrupt pgbench_accounts 14 1
/usr/local/var/postgres base/16384/25242 8192 13434880 1640
Reading byte 14 within file /usr/local/var/postgres/base/16384/25242
Current byte= 128 / $80
Modified byte= 129 / $81
File modified successfully
$ hexdump /usr/local/var/postgres/base/16384/25242 | head
0000000 00 00 00 00 00 00 00 00 00 00 04 00 0c 01 81 01

That doesn't impact selecting all of the rows:

$ psql -c "select count(*) from pgbench_accounts"
count
--------
100000

And pg_dump works fine against the table too. Tweaking this byte looks
like a reasonable first test case for seeing if checksums can catch an
error that query execution doesn't.

Next I'm going to test the functional part of the latest checksum patch;
duplicate Jeff's targeted performance tests; and then run some of my
own. I wanted to get this little tool circulating now that it's useful
first.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

New version of checksums patch.

Changes:
* rebased
* removed two duplicate lines; apparently the result of a bad merge
* Added heap page to WAL chain when logging an XLOG_HEAP2_VISIBLE to
avoid torn page issues updating PD_ALL_VISIBLE. This is the most
significant change.
* minor comment cleanup

No open issues that I'm aware of with the patch itself.

Greg appears to have made some progress on the automated corruption
tester.

Note to reviewers: I also have a patch out to remove PD_ALL_VISIBLE
entirely. The code around PD_ALL_VISIBLE is quite tricky (with or
without this patch), so if the PD_ALL_VISIBLE patch is committed first
then it will make reviewing this patch easier. Regardless, the second
patch to be committed will need to be rebased on top of the first.

Regards,
Jeff Davis

On Tue, 2013-01-15 at 19:36 -0500, Greg Smith wrote:
> First rev of a simple corruption program is attached, in very C-ish
> Python.

Great. Did you verify that my patch works as you expect at least in the
simple case?

> The parameters I settled on are to accept a relation name, byte
> offset, byte value, and what sort of operation to do: overwrite, AND,
> OR, XOR. I like XOR here because you can fix it just by running the
> program again.

Oh, good idea.

Regards,
Jeff Davis

On Wed, 2013-01-16 at 17:38 -0800, Jeff Davis wrote:
> New version of checksums patch.

And another new version of both patches.

Changes:
* Rebased.
* Rename SetBufferCommitInfoNeedsSave to MarkBufferDirtyHint. Now that
it's being used more places, it makes sense to give it a more generic
name.
* My colleague, Yingjie He, noticed that the FSM doesn't write any WAL,
and therefore we must protect those operations against torn pages. That
seems simple enough: just use MarkBufferDirtyHint (formerly
SetBufferCommitInfoNeedsSave) instead of MarkBufferDirty. The FSM
changes are not critical, so the fact that we may lose the dirty bit is
OK.

Regards,
Jeff Davis

On Thu, Jan 10, 2013 at 1:06 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Tue, 2012-12-04 at 01:03 -0800, Jeff Davis wrote:
>> For now, I rebased the patches against master, and did some very minor
>> cleanup.
>
> I think there is a problem here when setting PD_ALL_VISIBLE. I thought I
> had analyzed that before, but upon review, it doesn't look right.
> Setting PD_ALL_VISIBLE needs to be associated with a WAL action somehow,
> and a bumping of the LSN, otherwise there is a torn page hazard.
>
> The solution doesn't seem particularly difficult, but there are a few
> strange aspects and I'm not sure exactly which path I should take.
>
> First of all, the relationship between MarkBufferDirty and
> SetBufferCommitInfoNeedsSave is a little confusing. The comment over
> MarkBufferDirty is confusing because it says that the caller must have
> an exclusive lock, or else bad data could be written. But that doesn't
> have to do with marking the buffer dirty, that has to do with the data
> page change you make while you are marking it dirty -- if it's a single
> bit change, then there is no risk that I can see.
>
> In the current code, the only real code difference between the two is
> that SetBufferCommitInfoNeedsSave might fail to mark the buffer dirty if
> there is a race. So, in the current code, we could actually combine the
> two by passing a "force" flag (if true, behaves like MarkBufferDirty, if
> false, behaves like SetBufferCommitInfoNeedsSave).
>
> The checksums patch also introduces another behavior into
> SetBufferCommitInfoNeedsSave, which is to write an XLOG_HINT WAL record
> if checksums are enabled (to avoid torn page hazards). That's only
> necessary for changes where the caller does not write WAL itself and
> doesn't bump the LSN of the data page. (There's a reason the caller
> can't easily write the XLOG_HINT WAL itself.) So, we could introduce
> another flag "needsWAL" that would control whether we write the
> XLOG_HINT WAL or not (only applies with checksums on, of course).

I thought Simon had the idea, at some stage, of writing a WAL record
to cover hint-bit changes only at the time we *write* the buffer and
only if no FPI had already been emitted that checkpoint cycle. I'm
not sure whether that approach was sound, but if so it seems more
efficient than this approach.

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

On Fri, 2013-01-25 at 15:29 -0500, Robert Haas wrote:
> I thought Simon had the idea, at some stage, of writing a WAL record
> to cover hint-bit changes only at the time we *write* the buffer and
> only if no FPI had already been emitted that checkpoint cycle. I'm
> not sure whether that approach was sound, but if so it seems more
> efficient than this approach.

My patch is based on his original idea; although I've made quite a lot
of changes, I believe that I have stuck to his same basic design w.r.t.
WAL.

This patch does not cause a new FPI to be emitted if one has already
been emitted this cycle. It also does not emit a WAL record at all if an
FPI has already been emitted.

If we were to try to defer writing the WAL until the page was being
written, the most it would possibly save is the small XLOG_HINT WAL
record; it would not save any FPIs.

At first glance, it seems sound as long as the WAL FPI makes it to disk
before the data. But to meet that requirement, it seems like we'd need
to write an FPI and then immediately flush WAL before cleaning a page,
and that doesn't seem like a win. Do you (or Simon) see an opportunity
here that I'm missing?

By the way, the approach I took was to add the heap buffer to the WAL
chain of the XLOG_HEAP2_VISIBLE wal record when doing log_heap_visible.
It seemed simpler to understand than trying to add a bunch of options to
MarkBufferDirty.

Regards,
Jeff Davis

On Fri, Jan 25, 2013 at 9:35 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Fri, 2013-01-25 at 15:29 -0500, Robert Haas wrote:
>> I thought Simon had the idea, at some stage, of writing a WAL record
>> to cover hint-bit changes only at the time we *write* the buffer and
>> only if no FPI had already been emitted that checkpoint cycle. I'm
>> not sure whether that approach was sound, but if so it seems more
>> efficient than this approach.
>
> My patch is based on his original idea; although I've made quite a lot
> of changes, I believe that I have stuck to his same basic design w.r.t.
> WAL.
>
> This patch does not cause a new FPI to be emitted if one has already
> been emitted this cycle. It also does not emit a WAL record at all if an
> FPI has already been emitted.
>
> If we were to try to defer writing the WAL until the page was being
> written, the most it would possibly save is the small XLOG_HINT WAL
> record; it would not save any FPIs.

How is the XLOG_HINT_WAL record kept small and why does it not itself
require an FPI?

> At first glance, it seems sound as long as the WAL FPI makes it to disk
> before the data. But to meet that requirement, it seems like we'd need
> to write an FPI and then immediately flush WAL before cleaning a page,
> and that doesn't seem like a win. Do you (or Simon) see an opportunity
> here that I'm missing?

I am not sure that isn't a win. After all, we can need to flush WAL
before flushing a buffer anyway, so this is just adding another case -
and the payoff is that the initial access to a page, setting hint
bits, is quickly followed by a write operation, we avoid the need for
any extra WAL to cover the hint bit change. I bet that's common,
because if updating you'll usually need to look at the tuples on the
page and decide whether they are visible to your scan before, say,
updating one of them

> By the way, the approach I took was to add the heap buffer to the WAL
> chain of the XLOG_HEAP2_VISIBLE wal record when doing log_heap_visible.
> It seemed simpler to understand than trying to add a bunch of options to
> MarkBufferDirty.

Unless I am mistaken, that's going to heavy penalize the case where
the user vacuums an insert-only table. It will emit much more WAL
than currently.

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

On 25 January 2013 20:29, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> The checksums patch also introduces another behavior into
>> SetBufferCommitInfoNeedsSave, which is to write an XLOG_HINT WAL record
>> if checksums are enabled (to avoid torn page hazards). That's only
>> necessary for changes where the caller does not write WAL itself and
>> doesn't bump the LSN of the data page. (There's a reason the caller
>> can't easily write the XLOG_HINT WAL itself.) So, we could introduce
>> another flag "needsWAL" that would control whether we write the
>> XLOG_HINT WAL or not (only applies with checksums on, of course).
>
> I thought Simon had the idea, at some stage, of writing a WAL record
> to cover hint-bit changes only at the time we *write* the buffer and
> only if no FPI had already been emitted that checkpoint cycle. I'm
> not sure whether that approach was sound, but if so it seems more
> efficient than this approach.

The requirement is that we ensure that a FPI is written to WAL before
any changes to the block are made.

The patch does that by inserting an XLOG_HINT_WAL record when we set a
hint. The insert is a no-op if we've already written the FPI in this
checkpoint cycle and we don't even reach there except when dirtying a
clean data block.

If we attempted to defer the FPI last thing before write, we'd need to
cope with the case that writes at checkpoint occur after the logical
start of the checkpoint, and also with the overhead of additional
writes at checkpoint time.

I don't see any advantage in deferring the FPI, but I do see
disadvantage in complicating this.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jan 27, 2013 at 3:50 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> If we attempted to defer the FPI last thing before write, we'd need to
> cope with the case that writes at checkpoint occur after the logical
> start of the checkpoint, and also with the overhead of additional
> writes at checkpoint time.

Oh, good point. That's surely a good reason not to do it that way.

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

On Sat, 2013-01-26 at 23:23 -0500, Robert Haas wrote:
> > If we were to try to defer writing the WAL until the page was being
> > written, the most it would possibly save is the small XLOG_HINT WAL
> > record; it would not save any FPIs.
>
> How is the XLOG_HINT_WAL record kept small and why does it not itself
> require an FPI?

There's a maximum of one FPI per page per cycle, and we need the FPI for
any modified page in this design regardless.

So, deferring the XLOG_HINT WAL record doesn't change the total number
of FPIs emitted. The only savings would be on the trivial XLOG_HINT wal
record itself, because we might notice that it's not necessary in the
case where some other WAL action happened to the page.

> > At first glance, it seems sound as long as the WAL FPI makes it to disk
> > before the data. But to meet that requirement, it seems like we'd need
> > to write an FPI and then immediately flush WAL before cleaning a page,
> > and that doesn't seem like a win. Do you (or Simon) see an opportunity
> > here that I'm missing?
>
> I am not sure that isn't a win. After all, we can need to flush WAL
> before flushing a buffer anyway, so this is just adding another case -

Right, but if we get the WAL record in earlier, there is a greater
chance that it goes out with some unrelated WAL flush, and we don't need
to flush the WAL to clean the buffer at all. Separating WAL insertions
from WAL flushes seems like a fairly important goal, so I'm a little
skeptical of a proposal to narrow that gap so drastically.

It's hard to analyze without a specific proposal on the table. But if
cleaning pages requires a WAL record followed immediately by a flush, it
seems like that would increase the number of actual WAL flushes we need
to do by a lot.

> and the payoff is that the initial access to a page, setting hint
> bits, is quickly followed by a write operation, we avoid the need for
> any extra WAL to cover the hint bit change. I bet that's common,
> because if updating you'll usually need to look at the tuples on the
> page and decide whether they are visible to your scan before, say,
> updating one of them

That's a good point, I'm just not sure how avoid that problem without a
lot of complexity or a big cost. It seems like we want to defer the
XLOG_HINT WAL record for a short time; but not wait so long that we need
to clean the buffer or miss a chance to piggyback on another WAL flush.

> > By the way, the approach I took was to add the heap buffer to the WAL
> > chain of the XLOG_HEAP2_VISIBLE wal record when doing log_heap_visible.
> > It seemed simpler to understand than trying to add a bunch of options to
> > MarkBufferDirty.
>
> Unless I am mistaken, that's going to heavy penalize the case where
> the user vacuums an insert-only table. It will emit much more WAL
> than currently.

Yes, that's true, but I think that's pretty fundamental to this
checksums design (and of course it only applies if checksums are
enabled). We need to make sure an FPI is written and the LSN bumped
before we write a page.

That's why I was pushing a little on various proposals to either remove
or mitigate the impact of hint bits (load path, remove PD_ALL_VISIBLE,
cut down on the less-important hint bits, etc.). Maybe those aren't
viable, but that's why I spent time on them.

There are some other options, but I cringe a little bit thinking about
them. One is to simply exclude the PD_ALL_VISIBLE bit from the checksum
calculation, so that a torn page doesn't cause a problem (though
obviously that one bit would be vulnerable to corruption). Another is to
use a double-write buffer, but that didn't seem to go very far. Or, we
could abandon the whole thing and tell people to use ZFS/btrfs/NAS/SAN.

Regards,
Jeff Davis

On Sun, Jan 27, 2013 at 5:28 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> There's a maximum of one FPI per page per cycle, and we need the FPI for
> any modified page in this design regardless.
>
> So, deferring the XLOG_HINT WAL record doesn't change the total number
> of FPIs emitted. The only savings would be on the trivial XLOG_HINT wal
> record itself, because we might notice that it's not necessary in the
> case where some other WAL action happened to the page.

OK, I see. So the case where this really hurts is where a page is
updated for hint bits only and then not touched again for the
remainder of the checkpoint cycle.

>> > At first glance, it seems sound as long as the WAL FPI makes it to disk
>> > before the data. But to meet that requirement, it seems like we'd need
>> > to write an FPI and then immediately flush WAL before cleaning a page,
>> > and that doesn't seem like a win. Do you (or Simon) see an opportunity
>> > here that I'm missing?
>>
>> I am not sure that isn't a win. After all, we can need to flush WAL
>> before flushing a buffer anyway, so this is just adding another case -
>
> Right, but if we get the WAL record in earlier, there is a greater
> chance that it goes out with some unrelated WAL flush, and we don't need
> to flush the WAL to clean the buffer at all. Separating WAL insertions
> from WAL flushes seems like a fairly important goal, so I'm a little
> skeptical of a proposal to narrow that gap so drastically.
>
> It's hard to analyze without a specific proposal on the table. But if
> cleaning pages requires a WAL record followed immediately by a flush, it
> seems like that would increase the number of actual WAL flushes we need
> to do by a lot.

Yeah, maybe. I think Simon had a good argument for not pursuing this
route, anyway.

>> and the payoff is that the initial access to a page, setting hint
>> bits, is quickly followed by a write operation, we avoid the need for
>> any extra WAL to cover the hint bit change. I bet that's common,
>> because if updating you'll usually need to look at the tuples on the
>> page and decide whether they are visible to your scan before, say,
>> updating one of them
>
> That's a good point, I'm just not sure how avoid that problem without a
> lot of complexity or a big cost. It seems like we want to defer the
> XLOG_HINT WAL record for a short time; but not wait so long that we need
> to clean the buffer or miss a chance to piggyback on another WAL flush.
>
>> > By the way, the approach I took was to add the heap buffer to the WAL
>> > chain of the XLOG_HEAP2_VISIBLE wal record when doing log_heap_visible.
>> > It seemed simpler to understand than trying to add a bunch of options to
>> > MarkBufferDirty.
>>
>> Unless I am mistaken, that's going to heavy penalize the case where
>> the user vacuums an insert-only table. It will emit much more WAL
>> than currently.
>
> Yes, that's true, but I think that's pretty fundamental to this
> checksums design (and of course it only applies if checksums are
> enabled). We need to make sure an FPI is written and the LSN bumped
> before we write a page.
>
> That's why I was pushing a little on various proposals to either remove
> or mitigate the impact of hint bits (load path, remove PD_ALL_VISIBLE,
> cut down on the less-important hint bits, etc.). Maybe those aren't
> viable, but that's why I spent time on them.
>
> There are some other options, but I cringe a little bit thinking about
> them. One is to simply exclude the PD_ALL_VISIBLE bit from the checksum
> calculation, so that a torn page doesn't cause a problem (though
> obviously that one bit would be vulnerable to corruption). Another is to
> use a double-write buffer, but that didn't seem to go very far. Or, we
> could abandon the whole thing and tell people to use ZFS/btrfs/NAS/SAN.

I am inclined to think that we shouldn't do any of this stuff for now.
I think it's OK if the first version of checksums is
not-that-flexible and/or not-that-performant. We can optimize for
those things later. Trying to monkey with this at the same time we're
trying to get checksums in risks introducing new diverting focus from
getting checksums done at all, and risks also introducing new data
corruption bugs. We have a reputation of long standing for getting it
right first and then getting it to perform well later, so it shouldn't
a be a total shock if we take that approach here, too. I see no
reason to think that the performance problems must be solved up front
or not at all.

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

Attached is some bit rot updates to the checksums patches. The
replace-tli one still works fine. I fixed a number of conflicts in the
larger patch. The one I've attached here isn't 100% to project
standards--I don't have all the context diff tools setup yet for
example. I expect to revise this more now that I've got the whole week
cleared to work on CF submissions.

Here's the bit rot fixes:

src/backend/commands/vacuumlazy.c: Changed a call to look like this:

1157 visibilitymap_set(onerel, blkno, buffer,
InvalidXLogRecPtr, *vmbuffer,
1158 visibility_cutoff_xid);

To match calling order and make complier warnings go away.

src/backend/storage/buffer/bufmgr.c : merged against some changes
related to unlogged table buffer management. Merge seems clean once
done by hand.

src/include/catalog/pg_control.h: The patch used this value for XLOG_HINT:
#define XLOG_HINT 0x90

That's now been used for XLOG_END_OF_RECOVERY so I made it 0xA0 instead:
#define XLOG_HINT 0xA0

Unrelated to merge issues, I saw this in the patch:

localbuf.c: XXX do we want to write checksums for local buffers? An option?

And wanted to highlight this concern is still floating around.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Sun, Feb 24, 2013 at 10:30 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> Attached is some bit rot updates to the checksums patches. The replace-tli
> one still works fine....

I rather badly want this feature, and if the open issues with the
patch has hit zero, I'm thinking about applying it, shipping it, and
turning it on. Given that the heap format has not changed, the main
affordence I may check for is if I can work in backwards compatibility
(while not maintaining the checksums, of course) in case of an
emergency.

--
fdr

On 03/02/2013 12:48 AM, Daniel Farina wrote:
> On Sun, Feb 24, 2013 at 10:30 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>> Attached is some bit rot updates to the checksums patches. The replace-tli
>> one still works fine....
> I rather badly want this feature, and if the open issues with the
> patch has hit zero, I'm thinking about applying it, shipping it, and
> turning it on. Given that the heap format has not changed, the main
> affordence I may check for is if I can work in backwards compatibility
> (while not maintaining the checksums, of course) in case of an
> emergency.

Did you get a chance to see whether you can run it in
checksum-validation-and-update-off backward compatible mode? This seems
like an important thing to have working (and tested for) in case of
bugs, performance issues or other unforseen circumstances.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

The 16-bit checksum feature seems functional, with two sources of
overhead. There's some CPU time burned to compute checksums when pages
enter the system. And there's extra overhead for WAL logging hint bits.
I'll quantify both of those better in another message.

For completeness sake I've attached the latest versions of the patches I
tested (same set as my last message) along with the testing programs and
source changes that have been useful for my review. I have a test case
now demonstrating a tricky issue my gut told me was possible in page
header handling, and that's what I talk about most here.

= Handling bit errors in page headers =

The thing I've been stuck on is trying to find a case where turning
checksums on results in data that could be read becoming completely
unavailable, after a single bit of corruption. That seemed to me the
biggest risk of this feature. If checksumming can result in lost data,
where before that data would be available just with some potential for
error in it, that's kind of bad. I've created a program that does just
that, with a repeatable shell script test case (check-check.sh)

This builds on the example I gave before, where I can corrupt a single
bit of data in pgbench_accounts (lowest bit in byte 14 in the page) and
then reads that page without problems:

$ psql -c "select sum(abalance) from pgbench_accounts"
sum
-----
0

Corrupting the same bit on a checksums enabled build catches the problem:

WARNING: page verification failed, calculated checksum 5900 but
expected 9227
ERROR: invalid page header in block 0 of relation base/16384/16397

This is good, because it's exactly the sort of quiet corruption that the
feature is supposed to find. But clearly it's *possible* to still read
all of the data in this page, because the build without checksums does
just that. All of these fail now:

$ psql -c "select sum(abalance) from pgbench_accounts"
WARNING: page verification failed, calculated checksum 5900 but
expected 9227
ERROR: invalid page header in block 0 of relation base/16384/16397

$ psql -c "select * from pgbench_accounts"
WARNING: page verification failed, calculated checksum 5900 but
expected 9227
ERROR: invalid page header in block 0 of relation base/16384/16397

And you get this sort of mess out of pg_dump:

COPY pgbench_accounts (aid, bid, abalance, filler) FROM stdin;
pg_dump: WARNING: page verification failed, calculated checksum 5900
but expected 9227
\.

pg_dump: Dumping the contents of table "pgbench_accounts" failed:
PQgetResult() failed.
pg_dump: Error message from server: ERROR: invalid page header in block
0 of relation base/16384/16397
pg_dump: The command was: COPY public.pgbench_accounts (aid, bid,
abalance, filler) TO stdout;

I think an implicit goal of this feature was to soldier on when possible
to do so. The case where something in the page header is corrupted
seems the weakest part of that idea. I would still be happy to enable
this feature on a lot of servers, because stopping in the case of subtle
header corruption just means going to another known good copy of the
data; probably a standby server.

I could see some people getting surprised by this change though. I'm
not sure if it's possible to consider a checksum failure in a page
header something that is WARNed about, rather than always treating it as
a failure and the data is unavailable (without page inspection tools at
least). That seems like the main thing that might be improved in this
feature right now.

= Testing issues =

It is surprisingly hard to get a repeatable test program that corrupts a
bit on a data page. If you already have a copy of the page in memory
and you corrupt the copy on disk, the corrupted copy won't be noticed.
And if you happen to trigger a write of that page, the corruption will
quietly be fixed. This is all good, but it's something to be aware of
when writing test code.

The other thing to watch out for is that you're not hitting an
Index-Only Scan anywhere, because then you're bypassing the database
page you corrupted.

What I've done is come up with a repeatable test case that shows the
checksum patch finding a single bit of corruption that is missed by a
regular server. The program is named check-check.sh, and a full output
run is attached as check-check.log

I also added a developer only debugging test patch as
show_block_verifications.patch This makes every block read spew a
message about what relation it's touching, and proves the checksum
mechanism is being hit each time. The main reason I needed that is to
make sure the pages I expected to be read were actually the ones being
read. When I accidentally was hitting index-only scans for example, I
could tell that because it was touching something from
pgbench_accounts_pkey instead the pgbench_account table data I was
corrupting.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

And here's an updated version of the checksum corruption testing wrapper
script already. This includes an additional safety check that you've
set PGDATA to a location that can be erased. Presumably no one else
would like to accidentally do this:

rm -rf /*

Like I just did.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 12/19/12 6:30 PM, Jeff Davis wrote:
> I ran a few tests.
> Test 1 - find worst-case overhead for the checksum calculation on write:
> Test 2 - worst-case overhead for calculating checksum while reading data
> Test 3 - worst-case WAL overhead

What I've done is wrap all of these tests into a shell script that runs
them 3 times each, with and without checksums. That includes steps like
the spots where Jeff found a "sync" helped improve repeatability. I ran
these manually before and didn't notice enough of a difference to argue
with any of his results at the time. Having them packaged up usefully
means I can try some additional platforms too, and other testers should
be easily able to take a crack at it.

On the last one, in addition to runtime I directly measure how many
bytes of WAL are written. It's 0 in the usual case, where the hint bit
changes triggered by the first "SELECT * FROM foo" don't generate any WAL.

Detailed results with both my and Jeff's numbers are in the attached
spreadsheet. I did my tests on a Mac writing to SSD, to try and get
some variety in the test platforms. The main difference there is that
Test 1 is much slower on my system, enough so that the slowdown isn't as
pronounced.

Remember, these are a set of tests designed to magnify the worst case
here. I don't feel any of these results make the feature uncommittable.
The numbers I'm getting are not significantly different from the ones
Jeff posted back in December, and those were acceptable to some of the
early adopter candidates I've been surveying informally. These numbers
are amplifying overhead without doing much in the way of real disk I/O,
which can easily be a lot more expensive than any of this. I do think
there needs to be a bit more documentation of the potential downsides to
checksumming written though, since they are pretty hefty in some situations.

I'm going to get some pgbench results next, to try and put this into a
more realistic context too. The numbers for this round break down like
this:

= Test 1 - find worst-case overhead for the checksum calculation on write =

This can hit 25% of runtime when you isolate it out. I'm not sure if
how I'm running this multiple times makes sense yet. This one is so
much slower on my Mac that I can't barely see a change at all.

= Test 2 - worst-case overhead for calculating checksum while reading data =

Jeff saw an 18% slowdown, I get 24 to 32%. This one bothers me because
the hit is going to happen during the very common situation where data
is shuffling a lot between a larger OS cache and shared_buffers taking a
relatively small fraction. If that issue were cracked, such that
shared_buffers could be >50% of RAM, I think the typical real-world
impact of this would be easier to take.

= Test 3 - worst-case WAL overhead =

This is the really nasty one. The 10,000,000 rows touched by the SELECT
statement here create no WAL in a non-checksum environment. When
checksums are on, 368,513,656 bytes of WAL are written, so about 37
bytes per row.

Jeff saw this increase runtime by 135%, going from 1000ms to 2350ms. My
multiple runs are jumping around in a way I also don't trust fully yet.
But the first and best of the ones I'm seeing goes from 1660ms to
4013ms, which is a 140% increase. The others are even worse. I suspect
I'm filling a cache that isn't cleared before the second and third run
are over. I'll know for sure when I switch back to Linux.

The really nasty case I can see making people really cranky is where
someone has fsync on, a slowly rotating drive, and then discovers this
slowing read statements. There's already a decent share of "why is it
writing when I do 'SELECT *'?" complaints around the block I/O, which is
fully asynchronous in a lot of cases.

Right now the whole hint bit mechanism and its overhead are treated as
an internal detail that isn't in the regular documentation. I think
committing this sort of checksum patch will require exposing some of the
implementation to the user in the documentation, so people can
understand what the trouble cases are--either in advance or when trying
to puzzle out why they're hitting one of them.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 3/3/13 9:22 AM, Craig Ringer wrote:
> Did you get a chance to see whether you can run it in
> checksum-validation-and-update-off backward compatible mode? This seems
> like an important thing to have working (and tested for) in case of
> bugs, performance issues or other unforseen circumstances.

There isn't any way to do this in the current code. The big
simplification Jeff introduced here, to narrow complexity toward a
commit candidate, was to make checksumming a cluster-level decision.
You get it for everything or not at all.

The problem I posted about earlier today, where a header checksum error
can block access to the entire relation, could be resolved with some
sort of "ignore read checksums" GUC. But that's impractical right now
for the write side of things. There have been a long list of metadata
proposals to handle situations where part of a cluster is checksummed,
but not all of it. Once that sort of feature is implemented, it becomes
a lot easier to talk about selectively disabling writes.

As for a design of a GUC that might be useful here, the option itself
strikes me as being like archive_mode in its general use. There is an
element of parameters like wal_sync_method or enable_cassert though,
where the options available vary depending on how you built the cluster.
Maybe name it checksum_level with options like this:

off: only valid option if you didn't enable checksums with initdb
enforcing: full checksum behavior as written right now.
unvalidated: broken checksums on reads are ignored.

The main tricky case I see in that is where you read in a page with a
busted checksum using "unvalidated". Ideally you wouldn't write such a
page back out again, because it's going to hide that it's corrupted in
some way already. How to enforce that though? Perhaps "unvalidated"
only be allowed in a read-only transaction?

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 03/04/2013 11:18 AM, Greg Smith wrote:
> On 3/3/13 9:22 AM, Craig Ringer wrote:
>> Did you get a chance to see whether you can run it in
>> checksum-validation-and-update-off backward compatible mode? This seems
>> like an important thing to have working (and tested for) in case of
>> bugs, performance issues or other unforseen circumstances.
>
> There isn't any way to do this in the current code. The big
> simplification Jeff introduced here, to narrow complexity toward a
> commit candidate, was to make checksumming a cluster-level decision.
> You get it for everything or not at all.
>
> The problem I posted about earlier today, where a header checksum
> error can block access to the entire relation, could be resolved with
> some sort of "ignore read checksums" GUC. But that's impractical
> right now for the write side of things. There have been a long list
> of metadata proposals to handle situations where part of a cluster is
> checksummed, but not all of it. Once that sort of feature is
> implemented, it becomes a lot easier to talk about selectively
> disabling writes.
>
> As for a design of a GUC that might be useful here, the option itself
> strikes me as being like archive_mode in its general use. There is an
> element of parameters like wal_sync_method or enable_cassert though,
> where the options available vary depending on how you built the
> cluster. Maybe name it checksum_level with options like this:
>
> off: only valid option if you didn't enable checksums with initdb
> enforcing: full checksum behavior as written right now.
> unvalidated: broken checksums on reads are ignored.
>
> The main tricky case I see in that is where you read in a page with a
> busted checksum using "unvalidated". Ideally you wouldn't write such
> a page back out again, because it's going to hide that it's corrupted
> in some way already. How to enforce that though? Perhaps
> "unvalidated" only be allowed in a read-only transaction?
That sounds like a really good step for disaster recovery, yes.

I also suspect that at least in the first release it might be desirable
to have an option that essentially says "something's gone horribly wrong
and we no longer want to check or write checksums, we want a
non-checksummed DB that can still read our data from before we turned
checksumming off". Essentially, a way for someone who's trying
checksumming in production after their staging tests worked out OK to
abort and go back to the non-checksummed case without having to do a
full dump and reload.

Given that, I suspect we need a 4th state, like "disabled" or
"unvalidating_writable" where we ignore checksums completely and
maintain the checksum-enabled layout but just write padding to the
checksum fields and don't bother to check them on reading.

My key concern boils down to being able to get someone up and running
quickly and with minimal disruption if something we didn't think of goes
wrong. "Oh, you have to dump and reload your 1TB database before you can
start writing to it again" isn't going to cut it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 3/3/13 10:52 PM, Craig Ringer wrote:
> I also suspect that at least in the first release it might be desirable
> to have an option that essentially says "something's gone horribly wrong
> and we no longer want to check or write checksums, we want a
> non-checksummed DB that can still read our data from before we turned
> checksumming off".

I see that as being something that involves disabling the cluster-wide
flag that turns checksumming on, the one that is reported by
pg_controldata. I think it would have to be a one-way, system down kind
of change, which I think is fair given the ugly (but feasible) situation
you're describing. It would need to be something stronger than a GUC.
Once you start writing out pages without checksums, you're back into the
fuzzy state where some pages have them, others don't, and there's no
good way to deal with that yet.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 03/04/2013 12:19 PM, Greg Smith wrote:
> On 3/3/13 10:52 PM, Craig Ringer wrote:
>> I also suspect that at least in the first release it might be desirable
>> to have an option that essentially says "something's gone horribly wrong
>> and we no longer want to check or write checksums, we want a
>> non-checksummed DB that can still read our data from before we turned
>> checksumming off".
>
> I see that as being something that involves disabling the cluster-wide
> flag that turns checksumming on, the one that is reported by
> pg_controldata. I think it would have to be a one-way, system down
> kind of change, which I think is fair given the ugly (but feasible)
> situation you're describing. It would need to be something stronger
> than a GUC. Once you start writing out pages without checksums, you're
> back into the fuzzy state where some pages have them, others don't,
> and there's no good way to deal with that yet.

Agreed, I was envisioning a one-way process where re-enabling checksums
would involve be a re-initdb and reload. A DB restart seems perfectly
reasonable, it's just a full dump and reload before they can get running
again that I feel must be avoided.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 3 March 2013 18:24, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:

> The 16-bit checksum feature seems functional, with two sources of overhead.
> There's some CPU time burned to compute checksums when pages enter the
> system. And there's extra overhead for WAL logging hint bits. I'll
> quantify both of those better in another message.

It's crunch time. Do you and Jeff believe this patch should be
committed to Postgres core?

Are there objectors?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04.03.2013 09:11, Simon Riggs wrote:
> Are there objectors?

FWIW, I still think that checksumming belongs in the filesystem, not
PostgreSQL. If you go ahead with this anyway, at the very least I'd like
to see some sort of a comparison with e.g btrfs. How do performance,
error-detection rate, and behavior on error compare? Any other metrics
that are relevant here?

- Heikki

On Mon, 2013-03-04 at 10:36 +0200, Heikki Linnakangas wrote:
> On 04.03.2013 09:11, Simon Riggs wrote:
> > Are there objectors?
>
> FWIW, I still think that checksumming belongs in the filesystem, not
> PostgreSQL.

Doing checksums in the filesystem has some downsides. One is that you
need to use a copy-on-write filesystem like btrfs or zfs, which (by
design) will fragment the heap on random writes. If we're going to start
pushing people toward those systems, we will probably need to spend some
effort to mitigate this problem (aside: my patch to remove
PD_ALL_VISIBLE might get some new wind behind it).

There are also other issues, like what fraction of our users can freely
move to btrfs, and when. If it doesn't happen to be already there, you
need root to get it there, which has never been a requirement before.

I don't fundamentally disagree. We probably need to perform reasonably
well on btrfs in COW mode[1] regardless, because a lot of people will be
using it a few years from now. But there are a lot of unknowns here, and
I'm concerned about tying checksums to a series of things that will be
resolved a few years from now, if ever.

[1] Interestingly, you can turn off COW mode on btrfs, but you lose
checksums if you do.

> If you go ahead with this anyway, at the very least I'd like
> to see some sort of a comparison with e.g btrfs. How do performance,
> error-detection rate, and behavior on error compare? Any other metrics
> that are relevant here?

I suspect it will be hard to get an apples-to-apples comparison here
because of the heap fragmentation, which means that a sequential scan is
not so sequential. That may be acceptable for some workloads but not for
others, so it would get tricky to compare. And any performance numbers
from an experimental filesystem are somewhat suspect anyway.

Also, it's a little more challenging to test corruption on a filesystem,
because you need to find the location of the file you want to corrupt,
and corrupt it out from underneath the filesystem.

Greg may have more comments on this matter.

Regards,
Jeff Davis

On 3/4/13 2:11 AM, Simon Riggs wrote:
> It's crunch time. Do you and Jeff believe this patch should be
> committed to Postgres core?

I want to see a GUC to allow turning this off, to avoid the problem I
saw where a non-critical header corruption problem can cause an entire
page to be unreadable. A variation on that capable of turning this off
altogether, as Craig suggested, is a good idea too.

Those are both simple fixes, and I would be pleased to see this
committed at that point.

I'll write up a long discussion of filesystem trends and why I think
this is more relevant than ever if that's the main objection now. There
is no such thing as a stable release of btrfs, and no timetable for when
there will be one. I could do some benchmarks of that but I didn't
think they were very relevant. Who cares how fast something might run
when it may not work correctly? btrfs might as well be /dev/null to me
right now--sure it's fast, but maybe the data won't be there at all.
How long has it taken the Linux kernel to reach the point it handles
write barriers and fsync correctly? It does not give me a lot of
confidence that now is the time they'll suddenly start executing on
database filesystem mechanics perfectly.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Mon, 2013-02-25 at 01:30 -0500, Greg Smith wrote:
> Attached is some bit rot updates to the checksums patches. The
> replace-tli one still works fine. I fixed a number of conflicts in the
> larger patch. The one I've attached here isn't 100% to project
> standards--I don't have all the context diff tools setup yet for
> example. I expect to revise this more now that I've got the whole week
> cleared to work on CF submissions.

Thank you for the rebase. I redid the rebase myself and came up with
essentially the same result, but there was an additional problem that
needed fixing after the materialized view patch.

I will post a new version tonight that includes those fixes as well as
something to address these recent comments (probably just another GUC).
Further comment in another reply.

Regards,
Jeff Davis

On 04.03.2013 20:58, Greg Smith wrote:
> There
> is no such thing as a stable release of btrfs, and no timetable for when
> there will be one. I could do some benchmarks of that but I didn't think
> they were very relevant. Who cares how fast something might run when it
> may not work correctly? btrfs might as well be /dev/null to me right
> now--sure it's fast, but maybe the data won't be there at all.

This PostgreSQL patch hasn't seen any production use, either. In fact,
I'd consider btrfs to be more mature than this patch. Unless you think
that there will be some major changes to the worse in performance in
btrfs, it's perfectly valid and useful to compare the two.

A comparison with ZFS would be nice too. That's mature, and has checksums.

- Heikki

On Mon, 2013-03-04 at 11:52 +0800, Craig Ringer wrote:
> I also suspect that at least in the first release it might be desirable
> to have an option that essentially says "something's gone horribly wrong
> and we no longer want to check or write checksums, we want a
> non-checksummed DB that can still read our data from before we turned
> checksumming off". Essentially, a way for someone who's trying
> checksumming in production after their staging tests worked out OK to
> abort and go back to the non-checksummed case without having to do a
> full dump and reload.

A recovery option to extract data sounds like a good idea, but I don't
want to go as far as you are suggesting here.

An option to ignore checksum failures (while still printing the
warnings) sounds like all we need here. I think Greg's point that the
page might be written out again (hiding the corruption) is a very good
one, but the same is true for zero_damaged_pages. So we can just still
allow the writes to proceed (including setting the checksum on write),
and the system should be as available as it would be without checksums.

Regards,
Jeff Davis

On 04.03.2013 18:00, Jeff Davis wrote:
> On Mon, 2013-03-04 at 10:36 +0200, Heikki Linnakangas wrote:
>> On 04.03.2013 09:11, Simon Riggs wrote:
>>> Are there objectors?
>>
>> FWIW, I still think that checksumming belongs in the filesystem, not
>> PostgreSQL.
>
> Doing checksums in the filesystem has some downsides. One is that you
> need to use a copy-on-write filesystem like btrfs or zfs, which (by
> design) will fragment the heap on random writes.

Yeah, fragmentation will certainly hurt some workloads. But how badly,
and which workloads, and how does that compare with the work that
PostgreSQL has to do to maintain the checksums? I'd like to see some
data on those things.

> There are also other issues, like what fraction of our users can freely
> move to btrfs, and when. If it doesn't happen to be already there, you
> need root to get it there, which has never been a requirement before.

If you're serious enough about your data that you want checksums, you
should be able to choose your filesystem.

>> If you go ahead with this anyway, at the very least I'd like
>> to see some sort of a comparison with e.g btrfs. How do performance,
>> error-detection rate, and behavior on error compare? Any other metrics
>> that are relevant here?
>
> I suspect it will be hard to get an apples-to-apples comparison here
> because of the heap fragmentation, which means that a sequential scan is
> not so sequential. That may be acceptable for some workloads but not for
> others, so it would get tricky to compare.

An apples-to-apples comparison is to run the benchmark and see what
happens. If it gets fragmented as hell on btrfs, and performance tanks
because of that, then that's your result. If avoiding fragmentation is
critical to the workload, then with btrfs you'll want to run the
defragmenter in the background to keep it in order, and factor that into
the test case.

I realize that performance testing is laborious. But we can't skip it
and assume that the patch performs fine, because it's hard to benchmark.

- Heikki

On Mon, 2013-03-04 at 22:13 +0200, Heikki Linnakangas wrote:
> On 04.03.2013 20:58, Greg Smith wrote:
> > There
> > is no such thing as a stable release of btrfs, and no timetable for when
> > there will be one. I could do some benchmarks of that but I didn't think
> > they were very relevant. Who cares how fast something might run when it
> > may not work correctly? btrfs might as well be /dev/null to me right
> > now--sure it's fast, but maybe the data won't be there at all.
>
> This PostgreSQL patch hasn't seen any production use, either. In fact,
> I'd consider btrfs to be more mature than this patch. Unless you think
> that there will be some major changes to the worse in performance in
> btrfs, it's perfectly valid and useful to compare the two.
>
> A comparison with ZFS would be nice too. That's mature, and has checksums.

Is there any reason why we can't have both postgres and filesystem
checksums? The same user might not want both (or might, if neither are
entirely trustworthy yet), but I think it's too early to declare one as
the "right" solution and the other not. Even with btrfs stable, I
pointed out a number of reasons users might not want it, and reasons
that the project should not depend on it.

Numbers are always nice, but it takes a lot of effort to come up with
them. What kind of numbers are you looking for, and how *specifically*
will those numbers affect the decision?

If btrfs with checksums is 10% slower than ext4 with postgres checksums,
does that mean we should commit the postgres checksums?

On the other side of the coin, if btrfs with checksums is exactly the
same speed as ext4 with no postgres checksums (i.e. checksums are free
if we use btrfs), does that mean postgres checksums should be rejected?

Regards,
Jeff Davis

On Sun, 2013-03-03 at 22:18 -0500, Greg Smith wrote:
> As for a design of a GUC that might be useful here, the option itself
> strikes me as being like archive_mode in its general use. There is an
> element of parameters like wal_sync_method or enable_cassert though,
> where the options available vary depending on how you built the cluster.
> Maybe name it checksum_level with options like this:
>
> off: only valid option if you didn't enable checksums with initdb
> enforcing: full checksum behavior as written right now.
> unvalidated: broken checksums on reads are ignored.

I think GUCs should be orthogonal to initdb settings. If nothing else,
it's extra effort to get initdb to write the right postgresql.conf.

A single new GUC that prevents checksum failures from causing an error
seems sufficient to address the concerns you, Dan, and Craig raised.

We would still calculate the checksum and print the warning; and then
pass it through the rest of the header checks. If the header checks
pass, then it proceeds. If the header checks fail, and if
zero_damaged_pages is off, then it would still generate an error (as
today).

So: ignore_checksum_failures = on|off ?

> The main tricky case I see in that is where you read in a page with a
> busted checksum using "unvalidated". Ideally you wouldn't write such a
> page back out again, because it's going to hide that it's corrupted in
> some way already. How to enforce that though? Perhaps "unvalidated"
> only be allowed in a read-only transaction?

That's a good point. But we already have zero_damaged_pages, which does
something similar. And it's supposed to be a recovery option to get the
data out rather than something to run in online mode. It will still
print the warning, so it won't completely hide the corruption.

Regards,
Jeff Davis

On Mon, 2013-03-04 at 13:58 -0500, Greg Smith wrote:
> On 3/4/13 2:11 AM, Simon Riggs wrote:
> > It's crunch time. Do you and Jeff believe this patch should be
> > committed to Postgres core?
>
> I want to see a GUC to allow turning this off, to avoid the problem I
> saw where a non-critical header corruption problem can cause an entire
> page to be unreadable. A variation on that capable of turning this off
> altogether, as Craig suggested, is a good idea too.

Based on your comments as well those of Dan and Craig, I am leaning
toward a GUC that causes a checksum failure to be ignored. It will still
emit the checksum failure warning, but proceed.

That will then fall through to the normal header checks we've always
had, and the same zero_damaged_pages option. So, to get past a really
corrupt page, you'd need to set ignore_checksum_failure and
zero_damaged_pages.

> I'll write up a long discussion of filesystem trends and why I think
> this is more relevant than ever if that's the main objection now. There
> is no such thing as a stable release of btrfs, and no timetable for when
> there will be one. I could do some benchmarks of that but I didn't
> think they were very relevant. Who cares how fast something might run
> when it may not work correctly? btrfs might as well be /dev/null to me
> right now--sure it's fast, but maybe the data won't be there at all.
> How long has it taken the Linux kernel to reach the point it handles
> write barriers and fsync correctly? It does not give me a lot of
> confidence that now is the time they'll suddenly start executing on
> database filesystem mechanics perfectly.

I have a similar viewpoint here. It will take significant effort to come
up with anything, and I'm not sure how meaningful the numbers would be.
Even if btrfs is great, this feature is not mutually exclusive with
btrfs:
* users might not have easy access to run the filesystem
* they might not trust it
* they might get poor performance numbers
* postgres checksums might provide a good test of btrfs checksums, and
vice-versa, until both are stable

Additionally, I don't like the idea of depending so heavily on what
linux is doing. If there are performance problems that affect postgres,
will they fix them? Will they introduce new ones? Are there a zillion
tuneable options that a new user has to get right in order to run
postgres efficiently, and will poor settings mean a bunch of "postgres
is slow" blog posts?

Regards,
Jeff Davis

On 3/4/13 10:00 AM, Jeff Davis wrote:
> On Mon, 2013-03-04 at 10:36 +0200, Heikki Linnakangas wrote:
>> >On 04.03.2013 09:11, Simon Riggs wrote:
>>> > >Are there objectors?
>> >
>> >FWIW, I still think that checksumming belongs in the filesystem, not
>> >PostgreSQL.
> Doing checksums in the filesystem has some downsides.

Additionally, no filesystem I'm aware of checksums the data in the filesystem cache. A PG checksum would.

I'll also mention that this debate has been had in the past. The time to object to the concept of a checksuming feature was a long time ago, before a ton of development effort went into this... :(

On 3/4/13 2:48 PM, Jeff Davis wrote:
> On Mon, 2013-03-04 at 13:58 -0500, Greg Smith wrote:
>> >On 3/4/13 2:11 AM, Simon Riggs wrote:
>>> > >It's crunch time. Do you and Jeff believe this patch should be
>>> > >committed to Postgres core?
>> >
>> >I want to see a GUC to allow turning this off, to avoid the problem I
>> >saw where a non-critical header corruption problem can cause an entire
>> >page to be unreadable. A variation on that capable of turning this off
>> >altogether, as Craig suggested, is a good idea too.
> Based on your comments as well those of Dan and Craig, I am leaning
> toward a GUC that causes a checksum failure to be ignored. It will still
> emit the checksum failure warning, but proceed.

I suggest we paint that GUC along the lines of "checksum_failure_log_level", defaulting to ERROR. That way if someone wanted completely bury the elogs to like DEBUG they could.

My $2.98 (inflation adjusted).

On Mon, 2013-03-04 at 22:27 +0200, Heikki Linnakangas wrote:
> Yeah, fragmentation will certainly hurt some workloads. But how badly,
> and which workloads, and how does that compare with the work that
> PostgreSQL has to do to maintain the checksums? I'd like to see some
> data on those things.

I think we all would. Btrfs will be a major filesystem in a few years,
and we should be ready to support it.

Unfortunately, it's easier said than done. What you're talking about
seems like a significant benchmark report that encompasses a lot of
workloads. And there's a concern that a lot of it will be invalidated if
they are still improving the performance of btrfs.

> If you're serious enough about your data that you want checksums, you
> should be able to choose your filesystem.

I simply disagree. I am targeting my feature at casual users. They may
not have a lot of data or a dedicated DBA, but the data they do have
might be very important transactional data.

And right now, if they take a backup of their data, it will contain all
of the corruption from the original. And since corruption is silent
today, then they would probably think the backup is fine, and may delete
the previous good backups.