Re: Online verification of checksums

Hi,

v10 almost added online activation of checksums, but all we've got is
pg_verify_checksums, i.e. offline verification of checkums. 

However, we also got (online) checksum verification during base backups,
and I have ported/adapted David Steele's recheck code to my personal
fork of pg_checksums[1], removed the online check (for verification) and
that seems to work fine.

I've now forward-ported this change to pg_verify_checksums, in order to
make this application useful for online clusters, see attached patch.

I've tested this in a tight loop (while true; do pg_verify_checksums -D
data1 -d > /dev/null || /bin/true; done)[2] while doing "while true; do
createdb pgbench; pgbench -i -s 10 pgbench > /dev/null; dropdb pgbench;
done", which I already used to develop the original code in the fork and
which brought up a few bugs.

I got one checksums verification failure this way, all others were
caught by the recheck (I've introduced a 500ms delay for the first ten
failures) like this:

|pg_verify_checksums: checksum verification failed on first attempt in
|file "data1/base/16837/16850", block 7770: calculated checksum 785 but
|expected 5063
|pg_verify_checksums: block 7770 in file "data1/base/16837/16850"
|verified ok on recheck

However, I am also seeing sporadic (maybe 0.5 times per pgbench run)
failures like this:

|pg_verify_checksums: short read of block 2644 in file
|"data1/base/16637/16650", got only 4096 bytes

This is not strictly a verification failure, should we do anything about
this? In my fork, I am also rechecking on this[3] (and I am happy to
extend the patch that way), but that makes the code and the patch more
complicated and I wanted to check the general opinion on this case
first.

Michael

[1] https://github.com/credativ/pg_checksums/commit/dc052f0d6f1282d3c821
5b0eb28b8e7c4e74f9e5
[2] while patching out the somewhat unhelpful (in regular operation,
anyway) debug message for every successful checksum verification
[3] https://github.com/credativ/pg_checksums/blob/master/pg_checksums.c#
L160
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael(dot)banck(at)credativ(dot)de

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

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

On 26/07/2018 13:59, Michael Banck wrote:
> I've now forward-ported this change to pg_verify_checksums, in order to
> make this application useful for online clusters, see attached patch.

Why not provide this functionality as a server function or command.
Then you can access blocks with proper locks and don't have to do this
rather ad hoc retry logic on concurrent access.

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

On Thu, Aug 30, 2018 at 8:06 PM, Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:

> On 26/07/2018 13:59, Michael Banck wrote:
> > I've now forward-ported this change to pg_verify_checksums, in order to
> > make this application useful for online clusters, see attached patch.
>
> Why not provide this functionality as a server function or command.
> Then you can access blocks with proper locks and don't have to do this
> rather ad hoc retry logic on concurrent access.
>

I think it would make sense to provide this functionality in the "checksum
worker" infrastruture suggested in the online checksum enabling patch. But
I think being able to run it from the outside would also be useful,
particularly when it's this simple.

But why do we need a sleep in it? AFAICT this is basically the same code
that we have in basebackup.c, and that one does not need the sleep?
Certainly 500ms would be very long since we're just protecting against a
torn page, but the comment is wrong I think, and we're actually sleeping
0.5ms?

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

Hallo Michael,

> I've now forward-ported this change to pg_verify_checksums, in order to
> make this application useful for online clusters, see attached patch.

Patch does not seem to apply anymore, could you rebase it?

--
Fabien.

Hi,

The patch is mostly copying the verification / retry logic from
basebackup.c, but I think it omitted a rather important detail that
makes it incorrect in the presence of concurrent writes.

The very first thing basebackup does is this:

startptr = do_pg_start_backup(...);

i.e. it waits for a checkpoint, remembering the LSN. And then when
checking a page it does this:

if (!PageIsNew(page) && PageGetLSN(page) < startptr)
{
... verify the page checksum
}

Obviously, pg_verify_checksums can't do that easily because it's
supposed to run from outside the database instance. But the startptr
detail is pretty important because it supports this retry reasoning:

/*
* Retry the block on the first failure. It's
* possible that we read the first 4K page of the
* block just before postgres updated the entire block
* so it ends up looking torn to us. We only need to
* retry once because the LSN should be updated to
* something we can ignore on the next pass. If the
* error happens again then it is a true validation
* failure.
*/

Imagine the 8kB page as two 4kB pages, with the initial state being
[A1,A2] and another process over-writing it with [B1,B2]. If you read
the 8kB page, what states can you see?

I don't think POSIX provides any guarantees about atomicity of the write
calls (and even if it does, the filesystems on Linux don't seem to). So
you may observe both [A1,B2] or [A2,B1], or various inconsistent mixes
of the two versions, depending on timing. Well, torn pages ...

Pretty much the only thing you can rely on is that when one process does

write([B1,B2])

the other process may first read [A1,B2], but the next read will return
[B1,B2] (or possibly newer data, if there was another write). It will
not read the "stale" A1 again.

The basebackup relies on this kinda implicitly - on the retry it'll
notice the LSN changed (thanks to the startptr check), and the page will
be skipped entirely. This is pretty important, because the new page
might be torn in some other way.

The pg_verify_checksum apparently ignores this skip logic, because on
the retry it simply re-reads the page again, verifies the checksum and
reports an error. Which is broken, because the newly read page might be
torn again due to a concurrent write.

So IMHO this should do something similar to basebackup - check the page
LSN, and if it changed then skip the page.

I'm afraid this requires using the last checkpoint LSN, the way startptr
is used in basebackup. In particular we can't simply remember LSN from
the first read, because we might actually read [B1,A2] on the first try,
and then [B1,B2] or [B1,C2] on the retry. (Actually, the page may be
torn in various other ways, not necessarily at the 4kB boundary - it
might be torn right after the LSN, for example).

FWIW I also don't understand the purpose of pg_sleep(), it does not seem
to protect against anything, really.

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

Hi,

On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
> The patch is mostly copying the verification / retry logic from
> basebackup.c, but I think it omitted a rather important detail that
> makes it incorrect in the presence of concurrent writes.
>
> The very first thing basebackup does is this:
>
> startptr = do_pg_start_backup(...);
>
> i.e. it waits for a checkpoint, remembering the LSN. And then when
> checking a page it does this:
>
> if (!PageIsNew(page) && PageGetLSN(page) < startptr)
> {
> ... verify the page checksum
> }
>
> Obviously, pg_verify_checksums can't do that easily because it's
> supposed to run from outside the database instance.

It reads pg_control anyway, so couldn't we just take
ControlFile->checkPoint?

Other than that, basebackup.c seems to only look at pages which haven't
been changed since the backup starting checkpoint (see above if
statement). That's reasonable for backups, but is it just as reasonable
for online verification?

> But the startptr detail is pretty important because it supports this
> retry reasoning:
>
> /*
> * Retry the block on the first failure. It's
> * possible that we read the first 4K page of the
> * block just before postgres updated the entire block
> * so it ends up looking torn to us. We only need to
> * retry once because the LSN should be updated to
> * something we can ignore on the next pass. If the
> * error happens again then it is a true validation
> * failure.
> */
>
> Imagine the 8kB page as two 4kB pages, with the initial state being
> [A1,A2] and another process over-writing it with [B1,B2]. If you read
> the 8kB page, what states can you see?
>
> I don't think POSIX provides any guarantees about atomicity of the write
> calls (and even if it does, the filesystems on Linux don't seem to). So
> you may observe both [A1,B2] or [A2,B1], or various inconsistent mixes
> of the two versions, depending on timing. Well, torn pages ...
>
> Pretty much the only thing you can rely on is that when one process does
>
> write([B1,B2])
>
> the other process may first read [A1,B2], but the next read will return
> [B1,B2] (or possibly newer data, if there was another write). It will
> not read the "stale" A1 again.
>
> The basebackup relies on this kinda implicitly - on the retry it'll
> notice the LSN changed (thanks to the startptr check), and the page will
> be skipped entirely. This is pretty important, because the new page
> might be torn in some other way.
>
> The pg_verify_checksum apparently ignores this skip logic, because on
> the retry it simply re-reads the page again, verifies the checksum and
> reports an error. Which is broken, because the newly read page might be
> torn again due to a concurrent write.

Well, ok.

> So IMHO this should do something similar to basebackup - check the page
> LSN, and if it changed then skip the page.
>
> I'm afraid this requires using the last checkpoint LSN, the way startptr
> is used in basebackup. In particular we can't simply remember LSN from
> the first read, because we might actually read [B1,A2] on the first try,
> and then [B1,B2] or [B1,C2] on the retry. (Actually, the page may be
> torn in various other ways, not necessarily at the 4kB boundary - it
> might be torn right after the LSN, for example).

I'd prefer to come up with a plan where we don't just give up once we
see a new LSN, if possible. If I run a modified pg_verify_checksums
which skips on newer pages in a tight benchmark, basically everything
gets skipped as checkpoints don't happen often enough.

So how about we do check every page, but if one fails on retry, and the
LSN is newer than the checkpoint, we then skip it? Is that logic sound?

In any case, if we decide we really should skip the page if it is newer
than the checkpoint, I think it makes sense to track those skipped pages
and print their number out at the end, if there are any.

> FWIW I also don't understand the purpose of pg_sleep(), it does not seem
> to protect against anything, really.

Well, I've noticed that without it I get sporadic checksum failures on
reread, so I've added it to make them go away. It was certainly a
phenomenological decision that I am happy to trade for a better one.

Also, I noticed there's sometimes a 'data/global/pg_internal.init.606'
or some such file which pg_verify_checksums gets confused on, I guess we
should skip that as well. Can we assume that all files that start with
the ones in skip[] are safe to skip or should we have an exception for
files starting with pg_internal.init?

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
> > Obviously, pg_verify_checksums can't do that easily because it's
> > supposed to run from outside the database instance.
>
> It reads pg_control anyway, so couldn't we just take
> ControlFile->checkPoint?
>
> Other than that, basebackup.c seems to only look at pages which haven't
> been changed since the backup starting checkpoint (see above if
> statement). That's reasonable for backups, but is it just as reasonable
> for online verification?

Right, basebackup doesn't need to look at other pages.

> > The pg_verify_checksum apparently ignores this skip logic, because on
> > the retry it simply re-reads the page again, verifies the checksum and
> > reports an error. Which is broken, because the newly read page might be
> > torn again due to a concurrent write.
>
> Well, ok.

The newly read page will have an updated LSN though then on the re-read,
in which case basebackup can know that what happened was a rewrite of
the page and it no longer has to care about the page and can skip it.

I haven't looked, but if basebackup isn't checking the LSN again for the
newly read page then that'd be broken, but I believe it does (at least,
that's the algorithm we came up with for pgBackRest, and I know David
shared that when the basebackup code was being written).

> > So IMHO this should do something similar to basebackup - check the page
> > LSN, and if it changed then skip the page.
> >
> > I'm afraid this requires using the last checkpoint LSN, the way startptr
> > is used in basebackup. In particular we can't simply remember LSN from
> > the first read, because we might actually read [B1,A2] on the first try,
> > and then [B1,B2] or [B1,C2] on the retry. (Actually, the page may be
> > torn in various other ways, not necessarily at the 4kB boundary - it
> > might be torn right after the LSN, for example).
>
> I'd prefer to come up with a plan where we don't just give up once we
> see a new LSN, if possible. If I run a modified pg_verify_checksums
> which skips on newer pages in a tight benchmark, basically everything
> gets skipped as checkpoints don't happen often enough.

I'm really not sure how you expect to be able to do something different
here. Even if we started poking into shared buffers, all you'd be able
to see is that there's a bunch of dirty pages- and we don't maintain the
checksums in shared buffers, so it's not like you could verify them
there.

You could possibly have an option that says "force a checkpoint" but,
honestly, that's really not all that interesting either- all you'd be
doing is forcing all the pages to be written out from shared buffers
into the kernel cache and then reading them from there instead, it's not
like you'd actually be able to tell if there was a disk/storage error
because you'll only be looking at the kernel cache.

> So how about we do check every page, but if one fails on retry, and the
> LSN is newer than the checkpoint, we then skip it? Is that logic sound?

I thought that's what basebackup did- if it doesn't do that today, then
it really should.

> In any case, if we decide we really should skip the page if it is newer
> than the checkpoint, I think it makes sense to track those skipped pages
> and print their number out at the end, if there are any.

Not sure what the point of this is. If we wanted to really do something
to cross-check here, we'd track the pages that were skipped and then
look through the WAL to make sure that they're there. That's something
we've talked about doing with pgBackRest, but don't currently.

> > FWIW I also don't understand the purpose of pg_sleep(), it does not seem
> > to protect against anything, really.
>
> Well, I've noticed that without it I get sporadic checksum failures on
> reread, so I've added it to make them go away. It was certainly a
> phenomenological decision that I am happy to trade for a better one.

That then sounds like we really aren't re-checking the LSN, and we
really should be, to avoid getting these sporadic checksum failures on
reread..

> Also, I noticed there's sometimes a 'data/global/pg_internal.init.606'
> or some such file which pg_verify_checksums gets confused on, I guess we
> should skip that as well. Can we assume that all files that start with
> the ones in skip[] are safe to skip or should we have an exception for
> files starting with pg_internal.init?

Everything listed in skip is safe to skip on a restore.. I've not
really thought too much about if they're all safe to skip when checking
checksums for an online system, but I would generally think so..

Thanks!

Stephen

On 09/17/2018 04:46 PM, Stephen Frost wrote:
> Greetings,
>
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
>> On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
>>> Obviously, pg_verify_checksums can't do that easily because it's
>>> supposed to run from outside the database instance.
>>
>> It reads pg_control anyway, so couldn't we just take
>> ControlFile->checkPoint?
>>
>> Other than that, basebackup.c seems to only look at pages which haven't
>> been changed since the backup starting checkpoint (see above if
>> statement). That's reasonable for backups, but is it just as reasonable
>> for online verification?
>
> Right, basebackup doesn't need to look at other pages.
>
>>> The pg_verify_checksum apparently ignores this skip logic, because on
>>> the retry it simply re-reads the page again, verifies the checksum and
>>> reports an error. Which is broken, because the newly read page might be
>>> torn again due to a concurrent write.
>>
>> Well, ok.
>
> The newly read page will have an updated LSN though then on the re-read,
> in which case basebackup can know that what happened was a rewrite of
> the page and it no longer has to care about the page and can skip it.
>
> I haven't looked, but if basebackup isn't checking the LSN again for the
> newly read page then that'd be broken, but I believe it does (at least,
> that's the algorithm we came up with for pgBackRest, and I know David
> shared that when the basebackup code was being written).
>

Yes, basebackup does check the LSN on re-read, and skips the page if it
changed on re-read (because it eliminates the consistency guarantees
provided by the checkpoint).

>>> So IMHO this should do something similar to basebackup - check the page
>>> LSN, and if it changed then skip the page.
>>>
>>> I'm afraid this requires using the last checkpoint LSN, the way startptr
>>> is used in basebackup. In particular we can't simply remember LSN from
>>> the first read, because we might actually read [B1,A2] on the first try,
>>> and then [B1,B2] or [B1,C2] on the retry. (Actually, the page may be
>>> torn in various other ways, not necessarily at the 4kB boundary - it
>>> might be torn right after the LSN, for example).
>>
>> I'd prefer to come up with a plan where we don't just give up once we
>> see a new LSN, if possible. If I run a modified pg_verify_checksums
>> which skips on newer pages in a tight benchmark, basically everything
>> gets skipped as checkpoints don't happen often enough.
>
> I'm really not sure how you expect to be able to do something different
> here. Even if we started poking into shared buffers, all you'd be able
> to see is that there's a bunch of dirty pages- and we don't maintain the
> checksums in shared buffers, so it's not like you could verify them
> there.
>
> You could possibly have an option that says "force a checkpoint" but,
> honestly, that's really not all that interesting either- all you'd be
> doing is forcing all the pages to be written out from shared buffers
> into the kernel cache and then reading them from there instead, it's not
> like you'd actually be able to tell if there was a disk/storage error
> because you'll only be looking at the kernel cache.
>

Yeah.

>> So how about we do check every page, but if one fails on retry, and the
>> LSN is newer than the checkpoint, we then skip it? Is that logic sound?
>
> I thought that's what basebackup did- if it doesn't do that today, then
> it really should.
>

The crucial distinction here is that the trick is not in comparing LSNs
from the two page reads, but comparing it to the checkpoint LSN. If it's
greater, the page may be torn or broken, and there's no way to know
which case it is - so basebackup simply skips it.

>> In any case, if we decide we really should skip the page if it is newer
>> than the checkpoint, I think it makes sense to track those skipped pages
>> and print their number out at the end, if there are any.
>
> Not sure what the point of this is. If we wanted to really do something
> to cross-check here, we'd track the pages that were skipped and then
> look through the WAL to make sure that they're there. That's something
> we've talked about doing with pgBackRest, but don't currently.
>

I agree simply printing the page numbers seems rather useless. What we
could do is remember which pages we skipped and then try checking them
after another checkpoint. Or something like that.

>>> FWIW I also don't understand the purpose of pg_sleep(), it does not seem
>>> to protect against anything, really.
>>
>> Well, I've noticed that without it I get sporadic checksum failures on
>> reread, so I've added it to make them go away. It was certainly a
>> phenomenological decision that I am happy to trade for a better one.
>
> That then sounds like we really aren't re-checking the LSN, and we
> really should be, to avoid getting these sporadic checksum failures on
> reread..
>

Again, it's not enough to check the LSN against the preceding read. We
need a checkpoint LSN or something like that.

regards

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

On 09/17/2018 04:04 PM, Michael Banck wrote:
> Hi,
>
> On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
>> The patch is mostly copying the verification / retry logic from
>> basebackup.c, but I think it omitted a rather important detail that
>> makes it incorrect in the presence of concurrent writes.
>>
>> The very first thing basebackup does is this:
>>
>> startptr = do_pg_start_backup(...);
>>
>> i.e. it waits for a checkpoint, remembering the LSN. And then when
>> checking a page it does this:
>>
>> if (!PageIsNew(page) && PageGetLSN(page) < startptr)
>> {
>> ... verify the page checksum
>> }
>>
>> Obviously, pg_verify_checksums can't do that easily because it's
>> supposed to run from outside the database instance.
>
> It reads pg_control anyway, so couldn't we just take
> ControlFile->checkPoint?
>
> Other than that, basebackup.c seems to only look at pages which haven't
> been changed since the backup starting checkpoint (see above if
> statement). That's reasonable for backups, but is it just as reasonable
> for online verification?
>

I suppose we might refresh the checkpoint LSN regularly, and use the
most recent one. On large/busy databases that would allow checking
larger part of the database.

>> But the startptr detail is pretty important because it supports this
>> retry reasoning:
>>
>> /*
>> * Retry the block on the first failure. It's
>> * possible that we read the first 4K page of the
>> * block just before postgres updated the entire block
>> * so it ends up looking torn to us. We only need to
>> * retry once because the LSN should be updated to
>> * something we can ignore on the next pass. If the
>> * error happens again then it is a true validation
>> * failure.
>> */
>>
>> Imagine the 8kB page as two 4kB pages, with the initial state being
>> [A1,A2] and another process over-writing it with [B1,B2]. If you read
>> the 8kB page, what states can you see?
>>
>> I don't think POSIX provides any guarantees about atomicity of the write
>> calls (and even if it does, the filesystems on Linux don't seem to). So
>> you may observe both [A1,B2] or [A2,B1], or various inconsistent mixes
>> of the two versions, depending on timing. Well, torn pages ...
>>
>> Pretty much the only thing you can rely on is that when one process does
>>
>> write([B1,B2])
>>
>> the other process may first read [A1,B2], but the next read will return
>> [B1,B2] (or possibly newer data, if there was another write). It will
>> not read the "stale" A1 again.
>>
>> The basebackup relies on this kinda implicitly - on the retry it'll
>> notice the LSN changed (thanks to the startptr check), and the page will
>> be skipped entirely. This is pretty important, because the new page
>> might be torn in some other way.
>>
>> The pg_verify_checksum apparently ignores this skip logic, because on
>> the retry it simply re-reads the page again, verifies the checksum and
>> reports an error. Which is broken, because the newly read page might be
>> torn again due to a concurrent write.
>
> Well, ok.
>
>> So IMHO this should do something similar to basebackup - check the page
>> LSN, and if it changed then skip the page.
>>
>> I'm afraid this requires using the last checkpoint LSN, the way startptr
>> is used in basebackup. In particular we can't simply remember LSN from
>> the first read, because we might actually read [B1,A2] on the first try,
>> and then [B1,B2] or [B1,C2] on the retry. (Actually, the page may be
>> torn in various other ways, not necessarily at the 4kB boundary - it
>> might be torn right after the LSN, for example).
>
> I'd prefer to come up with a plan where we don't just give up once we
> see a new LSN, if possible. If I run a modified pg_verify_checksums
> which skips on newer pages in a tight benchmark, basically everything
> gets skipped as checkpoints don't happen often enough.
>

But in that case the checksums are verified when reading the buffer into
shared buffers, it's not like we don't notice the checksum error at all.
We are interested in the pages that have not been read/written for an
extended period time. So I think this is not a problem.

> So how about we do check every page, but if one fails on retry, and the
> LSN is newer than the checkpoint, we then skip it? Is that logic sound?
>

Hmmm, maybe.

> In any case, if we decide we really should skip the page if it is newer
> than the checkpoint, I think it makes sense to track those skipped pages
> and print their number out at the end, if there are any.
>

I agree it might be useful to know how many pages were skipped, and how
many actually passed the checksum check.

>> FWIW I also don't understand the purpose of pg_sleep(), it does not seem
>> to protect against anything, really.
>
> Well, I've noticed that without it I get sporadic checksum failures on
> reread, so I've added it to make them go away. It was certainly a
> phenomenological decision that I am happy to trade for a better one.
>

My guess is this happened because both the read and re-read completed
during the same write.

regards

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

Greetings,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 09/17/2018 04:46 PM, Stephen Frost wrote:
> > * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> >> On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
> >>> Obviously, pg_verify_checksums can't do that easily because it's
> >>> supposed to run from outside the database instance.
> >>
> >> It reads pg_control anyway, so couldn't we just take
> >> ControlFile->checkPoint?
> >>
> >> Other than that, basebackup.c seems to only look at pages which haven't
> >> been changed since the backup starting checkpoint (see above if
> >> statement). That's reasonable for backups, but is it just as reasonable
> >> for online verification?
> >
> > Right, basebackup doesn't need to look at other pages.
> >
> >>> The pg_verify_checksum apparently ignores this skip logic, because on
> >>> the retry it simply re-reads the page again, verifies the checksum and
> >>> reports an error. Which is broken, because the newly read page might be
> >>> torn again due to a concurrent write.
> >>
> >> Well, ok.
> >
> > The newly read page will have an updated LSN though then on the re-read,
> > in which case basebackup can know that what happened was a rewrite of
> > the page and it no longer has to care about the page and can skip it.
> >
> > I haven't looked, but if basebackup isn't checking the LSN again for the
> > newly read page then that'd be broken, but I believe it does (at least,
> > that's the algorithm we came up with for pgBackRest, and I know David
> > shared that when the basebackup code was being written).
>
> Yes, basebackup does check the LSN on re-read, and skips the page if it
> changed on re-read (because it eliminates the consistency guarantees
> provided by the checkpoint).

Ok, good, though I'm not sure what you mean by 'eliminates the
consistency guarantees provided by the checkpoint'. The point is that
the page will be in the WAL and the WAL will be replayed during the
restore of the backup.

> >> So how about we do check every page, but if one fails on retry, and the
> >> LSN is newer than the checkpoint, we then skip it? Is that logic sound?
> >
> > I thought that's what basebackup did- if it doesn't do that today, then
> > it really should.
>
> The crucial distinction here is that the trick is not in comparing LSNs
> from the two page reads, but comparing it to the checkpoint LSN. If it's
> greater, the page may be torn or broken, and there's no way to know
> which case it is - so basebackup simply skips it.

Sure, because we don't care about it any longer- that page isn't
interesting because the WAL will replay over it. IIRC it actually goes
something like: check the checksum, if it failed then check if the LSN
is greater than the checkpoint (of the backup start..), if not, then
re-read, if the LSN is now newer than the checkpoint then skip, if the
LSN is the same then throw an error.

> >> In any case, if we decide we really should skip the page if it is newer
> >> than the checkpoint, I think it makes sense to track those skipped pages
> >> and print their number out at the end, if there are any.
> >
> > Not sure what the point of this is. If we wanted to really do something
> > to cross-check here, we'd track the pages that were skipped and then
> > look through the WAL to make sure that they're there. That's something
> > we've talked about doing with pgBackRest, but don't currently.
>
> I agree simply printing the page numbers seems rather useless. What we
> could do is remember which pages we skipped and then try checking them
> after another checkpoint. Or something like that.

I'm still not sure I'm seeing the point of that. They're still going to
almost certainly be in the kernel cache. The reason for checking
against the WAL would be to detect errors in PG where we aren't putting
a page into the WAL when it really should be, or something similar,
which seems like it at least could be useful.

Maybe to put it another way- there's very little point in checking the
checksum of a page which we know must be re-written during recovery to
get to a consistent point. I don't think it hurts in the general case,
but I wouldn't write a lot of code which then needs to be tested to
handle it. I also don't think that we really need to make
pg_verify_checksum spend lots of extra cycles trying to verify that
*every* page had its checksum validated when we know that lots of pages
are going to be in memory marked dirty and our checking of them will be
ultimately pointless as they'll either be written out by the
checkpointer or some other process, or we'll replay them from the WAL if
we crash.

> >>> FWIW I also don't understand the purpose of pg_sleep(), it does not seem
> >>> to protect against anything, really.
> >>
> >> Well, I've noticed that without it I get sporadic checksum failures on
> >> reread, so I've added it to make them go away. It was certainly a
> >> phenomenological decision that I am happy to trade for a better one.
> >
> > That then sounds like we really aren't re-checking the LSN, and we
> > really should be, to avoid getting these sporadic checksum failures on
> > reread..
>
> Again, it's not enough to check the LSN against the preceding read. We
> need a checkpoint LSN or something like that.

I actually tend to disagree with you that, for this purpose, it's
actually necessary to check against the checkpoint LSN- if the LSN
changed and everything is operating correctly then the new LSN must be
more recent than the last checkpoint location or things are broken
badly.

Now, that said, I do think it's a good *idea* to check against the
checkpoint LSN (presuming this is for online checking of checksums- for
basebackup, we could just check against the backup-start LSN as anything
after that point will be rewritten by WAL anyway). The reason that I
think it's a good idea to check against the checkpoint LSN is that we'd
want to throw a big warning if the kernel is just feeding us random
garbage on reads and only finding a difference between two reads isn't
really doing any kind of validation, whereas checking against the
checkpoint-LSN would at least give us some idea that the value being
read isn't completely ridiculous.

When it comes to if the pg_sleep() is necessary or not, I have to admit
to being unsure about that.. I could see how it might be but it seems a
bit surprising- I'd probably want to see exactly what the page was at
the time of the failure and at the time of the second (no-sleep) re-read
and then after a delay and convince myself that it was just an unlucky
case of being scheduled in twice to read that page before the process
writing it out got a chance to finish the write.

Thanks!

Stephen

Hi,

On 09/17/2018 06:42 PM, Stephen Frost wrote:
> Greetings,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
>> On 09/17/2018 04:46 PM, Stephen Frost wrote:
>>> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
>>>> On Mon, Sep 03, 2018 at 10:29:18PM +0200, Tomas Vondra wrote:
>>>>> Obviously, pg_verify_checksums can't do that easily because it's
>>>>> supposed to run from outside the database instance.
>>>>
>>>> It reads pg_control anyway, so couldn't we just take
>>>> ControlFile->checkPoint?
>>>>
>>>> Other than that, basebackup.c seems to only look at pages which haven't
>>>> been changed since the backup starting checkpoint (see above if
>>>> statement). That's reasonable for backups, but is it just as reasonable
>>>> for online verification?
>>>
>>> Right, basebackup doesn't need to look at other pages.
>>>
>>>>> The pg_verify_checksum apparently ignores this skip logic, because on
>>>>> the retry it simply re-reads the page again, verifies the checksum and
>>>>> reports an error. Which is broken, because the newly read page might be
>>>>> torn again due to a concurrent write.
>>>>
>>>> Well, ok.
>>>
>>> The newly read page will have an updated LSN though then on the re-read,
>>> in which case basebackup can know that what happened was a rewrite of
>>> the page and it no longer has to care about the page and can skip it.
>>>
>>> I haven't looked, but if basebackup isn't checking the LSN again for the
>>> newly read page then that'd be broken, but I believe it does (at least,
>>> that's the algorithm we came up with for pgBackRest, and I know David
>>> shared that when the basebackup code was being written).
>>
>> Yes, basebackup does check the LSN on re-read, and skips the page if it
>> changed on re-read (because it eliminates the consistency guarantees
>> provided by the checkpoint).
>
> Ok, good, though I'm not sure what you mean by 'eliminates the
> consistency guarantees provided by the checkpoint'. The point is that
> the page will be in the WAL and the WAL will be replayed during the
> restore of the backup.
>

The checkpoint guarantees that the whole page was written and flushed to
disk with an LSN before the ckeckpoint LSN. So when you read a page with
that LSN, you know the whole write already completed and a read won't
return data from before the LSN.

Without the checkpoint that's not guaranteed, and simply re-reading the
page and rechecking it vs. the first read does not help:

1) write the first 512B of the page (sector), which includes the LSN

2) read the whole page, which will be a mix [new 512B, ... old ... ]

3) the checksum verification fails

4) read the page again (possibly reading a bit more new data)

5) the LSN did not change compared to the first read, yet the checksum
still fails

>>>> So how about we do check every page, but if one fails on retry, and the
>>>> LSN is newer than the checkpoint, we then skip it? Is that logic sound?
>>>
>>> I thought that's what basebackup did- if it doesn't do that today, then
>>> it really should.
>>
>> The crucial distinction here is that the trick is not in comparing LSNs
>> from the two page reads, but comparing it to the checkpoint LSN. If it's
>> greater, the page may be torn or broken, and there's no way to know
>> which case it is - so basebackup simply skips it.
>
> Sure, because we don't care about it any longer- that page isn't
> interesting because the WAL will replay over it. IIRC it actually goes
> something like: check the checksum, if it failed then check if the LSN
> is greater than the checkpoint (of the backup start..), if not, then
> re-read, if the LSN is now newer than the checkpoint then skip, if the
> LSN is the same then throw an error.
>

Nope, we only verify the checksum if it's LSN precedes the checkpoint:

https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454

>>>> In any case, if we decide we really should skip the page if it is newer
>>>> than the checkpoint, I think it makes sense to track those skipped pages
>>>> and print their number out at the end, if there are any.
>>>
>>> Not sure what the point of this is. If we wanted to really do something
>>> to cross-check here, we'd track the pages that were skipped and then
>>> look through the WAL to make sure that they're there. That's something
>>> we've talked about doing with pgBackRest, but don't currently.
>>
>> I agree simply printing the page numbers seems rather useless. What we
>> could do is remember which pages we skipped and then try checking them
>> after another checkpoint. Or something like that.
>
> I'm still not sure I'm seeing the point of that. They're still going to
> almost certainly be in the kernel cache. The reason for checking
> against the WAL would be to detect errors in PG where we aren't putting
> a page into the WAL when it really should be, or something similar,
> which seems like it at least could be useful.
>
> Maybe to put it another way- there's very little point in checking the
> checksum of a page which we know must be re-written during recovery to
> get to a consistent point. I don't think it hurts in the general case,
> but I wouldn't write a lot of code which then needs to be tested to
> handle it. I also don't think that we really need to make
> pg_verify_checksum spend lots of extra cycles trying to verify that
> *every* page had its checksum validated when we know that lots of pages
> are going to be in memory marked dirty and our checking of them will be
> ultimately pointless as they'll either be written out by the
> checkpointer or some other process, or we'll replay them from the WAL if
> we crash.
>

Yeah, I agree.

>>>>> FWIW I also don't understand the purpose of pg_sleep(), it does not seem
>>>>> to protect against anything, really.
>>>>
>>>> Well, I've noticed that without it I get sporadic checksum failures on
>>>> reread, so I've added it to make them go away. It was certainly a
>>>> phenomenological decision that I am happy to trade for a better one.
>>>
>>> That then sounds like we really aren't re-checking the LSN, and we
>>> really should be, to avoid getting these sporadic checksum failures on
>>> reread..
>>
>> Again, it's not enough to check the LSN against the preceding read. We
>> need a checkpoint LSN or something like that.
>
> I actually tend to disagree with you that, for this purpose, it's
> actually necessary to check against the checkpoint LSN- if the LSN
> changed and everything is operating correctly then the new LSN must be
> more recent than the last checkpoint location or things are broken
> badly.
>

I don't follow. Are you suggesting we don't need the checkpoint LSN?

I'm pretty sure that's not the case. The thing is - the LSN may not
change between the two reads, but that's not a guarantee the page was
not torn. The example I posted earlier in this message illustrates that.

> Now, that said, I do think it's a good *idea* to check against the
> checkpoint LSN (presuming this is for online checking of checksums- for
> basebackup, we could just check against the backup-start LSN as anything
> after that point will be rewritten by WAL anyway). The reason that I
> think it's a good idea to check against the checkpoint LSN is that we'd
> want to throw a big warning if the kernel is just feeding us random
> garbage on reads and only finding a difference between two reads isn't
> really doing any kind of validation, whereas checking against the
> checkpoint-LSN would at least give us some idea that the value being
> read isn't completely ridiculous.
>
> When it comes to if the pg_sleep() is necessary or not, I have to admit
> to being unsure about that.. I could see how it might be but it seems a
> bit surprising- I'd probably want to see exactly what the page was at
> the time of the failure and at the time of the second (no-sleep) re-read
> and then after a delay and convince myself that it was just an unlucky
> case of being scheduled in twice to read that page before the process
> writing it out got a chance to finish the write.
>

I think the pg_sleep() is a pretty strong sign there's something broken.
At the very least, it's likely to misbehave on machines with different
timings, machines under memory and/or memory pressure, etc.

regards

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

Greetings,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 09/17/2018 06:42 PM, Stephen Frost wrote:
> > Ok, good, though I'm not sure what you mean by 'eliminates the
> > consistency guarantees provided by the checkpoint'. The point is that
> > the page will be in the WAL and the WAL will be replayed during the
> > restore of the backup.
>
> The checkpoint guarantees that the whole page was written and flushed to
> disk with an LSN before the ckeckpoint LSN. So when you read a page with
> that LSN, you know the whole write already completed and a read won't
> return data from before the LSN.

Well, you know that the first part was written out at some prior point,
but you could end up reading the first part of a page with an older LSN
while also reading the second part with new data.

> Without the checkpoint that's not guaranteed, and simply re-reading the
> page and rechecking it vs. the first read does not help:
>
> 1) write the first 512B of the page (sector), which includes the LSN
>
> 2) read the whole page, which will be a mix [new 512B, ... old ... ]
>
> 3) the checksum verification fails
>
> 4) read the page again (possibly reading a bit more new data)
>
> 5) the LSN did not change compared to the first read, yet the checksum
> still fails

So, I agree with all of the above though I've found it to be extremely
rare to get a single read which you've managed to catch part-way through
a write, getting multiple of them over a period of time strikes me as
even more unlikely. Still, if we can come up with a solution to solve
all of this, great, but I'm not sure that I'm hearing one.

> > Sure, because we don't care about it any longer- that page isn't
> > interesting because the WAL will replay over it. IIRC it actually goes
> > something like: check the checksum, if it failed then check if the LSN
> > is greater than the checkpoint (of the backup start..), if not, then
> > re-read, if the LSN is now newer than the checkpoint then skip, if the
> > LSN is the same then throw an error.
>
> Nope, we only verify the checksum if it's LSN precedes the checkpoint:
>
> https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454

That seems like it's leaving something on the table, but, to be fair, we
know that all of those pages should be rewritten by WAL anyway so they
aren't all that interesting to us, particularly in the basebackup case.

> > I actually tend to disagree with you that, for this purpose, it's
> > actually necessary to check against the checkpoint LSN- if the LSN
> > changed and everything is operating correctly then the new LSN must be
> > more recent than the last checkpoint location or things are broken
> > badly.
>
> I don't follow. Are you suggesting we don't need the checkpoint LSN?
>
> I'm pretty sure that's not the case. The thing is - the LSN may not
> change between the two reads, but that's not a guarantee the page was
> not torn. The example I posted earlier in this message illustrates that.

I agree that there's some risk there, but it's certainly much less
likely.

> > Now, that said, I do think it's a good *idea* to check against the
> > checkpoint LSN (presuming this is for online checking of checksums- for
> > basebackup, we could just check against the backup-start LSN as anything
> > after that point will be rewritten by WAL anyway). The reason that I
> > think it's a good idea to check against the checkpoint LSN is that we'd
> > want to throw a big warning if the kernel is just feeding us random
> > garbage on reads and only finding a difference between two reads isn't
> > really doing any kind of validation, whereas checking against the
> > checkpoint-LSN would at least give us some idea that the value being
> > read isn't completely ridiculous.
> >
> > When it comes to if the pg_sleep() is necessary or not, I have to admit
> > to being unsure about that.. I could see how it might be but it seems a
> > bit surprising- I'd probably want to see exactly what the page was at
> > the time of the failure and at the time of the second (no-sleep) re-read
> > and then after a delay and convince myself that it was just an unlucky
> > case of being scheduled in twice to read that page before the process
> > writing it out got a chance to finish the write.
>
> I think the pg_sleep() is a pretty strong sign there's something broken.
> At the very least, it's likely to misbehave on machines with different
> timings, machines under memory and/or memory pressure, etc.

If we assume that what you've outlined above is a serious enough issue
that we have to address it, and do so without a pg_sleep(), then I think
we have to bake into this a way for the process to check with PG as to
what the page's current LSN is, in shared buffers, because that's the
only place where we've got the locking required to ensure that we don't
end up with a read of a partially written page, and I'm really not
entirely convinced that we need to go to that level. It'd certainly add
a huge amount of additional complexity for what appears to be a quite
unlikely gain.

I'll chat w/ David shortly about this again though and get his thoughts
on it. This is certainly an area we've spent time thinking about but
are obviously also open to finding a better solution.

Thanks!

Stephen

On 09/17/2018 07:11 PM, Stephen Frost wrote:
> Greetings,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
>> On 09/17/2018 06:42 PM, Stephen Frost wrote:
>>> Ok, good, though I'm not sure what you mean by 'eliminates the
>>> consistency guarantees provided by the checkpoint'. The point is that
>>> the page will be in the WAL and the WAL will be replayed during the
>>> restore of the backup.
>>
>> The checkpoint guarantees that the whole page was written and flushed to
>> disk with an LSN before the ckeckpoint LSN. So when you read a page with
>> that LSN, you know the whole write already completed and a read won't
>> return data from before the LSN.
>
> Well, you know that the first part was written out at some prior point,
> but you could end up reading the first part of a page with an older LSN
> while also reading the second part with new data.
>

Doesn't the checkpoint fsync pretty much guarantee this can't happen?

>> Without the checkpoint that's not guaranteed, and simply re-reading the
>> page and rechecking it vs. the first read does not help:
>>
>> 1) write the first 512B of the page (sector), which includes the LSN
>>
>> 2) read the whole page, which will be a mix [new 512B, ... old ... ]
>>
>> 3) the checksum verification fails
>>
>> 4) read the page again (possibly reading a bit more new data)
>>
>> 5) the LSN did not change compared to the first read, yet the checksum
>> still fails
>
> So, I agree with all of the above though I've found it to be extremely
> rare to get a single read which you've managed to catch part-way through
> a write, getting multiple of them over a period of time strikes me as
> even more unlikely. Still, if we can come up with a solution to solve
> all of this, great, but I'm not sure that I'm hearing one.
>

I don't recall claiming catching many such torn pages - I'm sure it's
not very common in most workloads. But I suspect constructing workloads
hitting them regularly is not very difficult either (something with a
lot of churn in shared buffers should do the trick).

>>> Sure, because we don't care about it any longer- that page isn't
>>> interesting because the WAL will replay over it. IIRC it actually goes
>>> something like: check the checksum, if it failed then check if the LSN
>>> is greater than the checkpoint (of the backup start..), if not, then
>>> re-read, if the LSN is now newer than the checkpoint then skip, if the
>>> LSN is the same then throw an error.
>>
>> Nope, we only verify the checksum if it's LSN precedes the checkpoint:
>>
>> https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454
>
> That seems like it's leaving something on the table, but, to be fair, we
> know that all of those pages should be rewritten by WAL anyway so they
> aren't all that interesting to us, particularly in the basebackup case.
>

Yep.

>>> I actually tend to disagree with you that, for this purpose, it's
>>> actually necessary to check against the checkpoint LSN- if the LSN
>>> changed and everything is operating correctly then the new LSN must be
>>> more recent than the last checkpoint location or things are broken
>>> badly.
>>
>> I don't follow. Are you suggesting we don't need the checkpoint LSN?
>>
>> I'm pretty sure that's not the case. The thing is - the LSN may not
>> change between the two reads, but that's not a guarantee the page was
>> not torn. The example I posted earlier in this message illustrates that.
>
> I agree that there's some risk there, but it's certainly much less
> likely.
>

Well. If we're going to report a checksum failure, we better be sure it
actually is a broken page. I don't want users to start chasing bogus
data corruption issues.

>>> Now, that said, I do think it's a good *idea* to check against the
>>> checkpoint LSN (presuming this is for online checking of checksums- for
>>> basebackup, we could just check against the backup-start LSN as anything
>>> after that point will be rewritten by WAL anyway). The reason that I
>>> think it's a good idea to check against the checkpoint LSN is that we'd
>>> want to throw a big warning if the kernel is just feeding us random
>>> garbage on reads and only finding a difference between two reads isn't
>>> really doing any kind of validation, whereas checking against the
>>> checkpoint-LSN would at least give us some idea that the value being
>>> read isn't completely ridiculous.
>>>
>>> When it comes to if the pg_sleep() is necessary or not, I have to admit
>>> to being unsure about that.. I could see how it might be but it seems a
>>> bit surprising- I'd probably want to see exactly what the page was at
>>> the time of the failure and at the time of the second (no-sleep) re-read
>>> and then after a delay and convince myself that it was just an unlucky
>>> case of being scheduled in twice to read that page before the process
>>> writing it out got a chance to finish the write.
>>
>> I think the pg_sleep() is a pretty strong sign there's something broken.
>> At the very least, it's likely to misbehave on machines with different
>> timings, machines under memory and/or memory pressure, etc.
>
> If we assume that what you've outlined above is a serious enough issue
> that we have to address it, and do so without a pg_sleep(), then I think
> we have to bake into this a way for the process to check with PG as to
> what the page's current LSN is, in shared buffers, because that's the
> only place where we've got the locking required to ensure that we don't
> end up with a read of a partially written page, and I'm really not
> entirely convinced that we need to go to that level. It'd certainly add
> a huge amount of additional complexity for what appears to be a quite
> unlikely gain.
>
> I'll chat w/ David shortly about this again though and get his thoughts
> on it. This is certainly an area we've spent time thinking about but
> are obviously also open to finding a better solution.
>

Why not to simply look at the last checkpoint LSN and use that the same
way basebackup does? AFAICS that should make the pg_sleep() unnecessary.

regards

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

Greetings,

On Mon, Sep 17, 2018 at 13:20 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

> On 09/17/2018 07:11 PM, Stephen Frost wrote:
> > Greetings,
> >
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> >> On 09/17/2018 06:42 PM, Stephen Frost wrote:
> >>> Ok, good, though I'm not sure what you mean by 'eliminates the
> >>> consistency guarantees provided by the checkpoint'. The point is that
> >>> the page will be in the WAL and the WAL will be replayed during the
> >>> restore of the backup.
> >>
> >> The checkpoint guarantees that the whole page was written and flushed to
> >> disk with an LSN before the ckeckpoint LSN. So when you read a page with
> >> that LSN, you know the whole write already completed and a read won't
> >> return data from before the LSN.
> >
> > Well, you know that the first part was written out at some prior point,
> > but you could end up reading the first part of a page with an older LSN
> > while also reading the second part with new data.

Doesn't the checkpoint fsync pretty much guarantee this can't happen?

How? Either it’s possible for the latter half of a page to be updated
before the first half (where the LSN lives), or it isn’t. If it’s possible
then that LSN could be ancient and it wouldn’t matter.

>> Without the checkpoint that's not guaranteed, and simply re-reading the
> >> page and rechecking it vs. the first read does not help:
> >>
> >> 1) write the first 512B of the page (sector), which includes the LSN
> >>
> >> 2) read the whole page, which will be a mix [new 512B, ... old ... ]
> >>
> >> 3) the checksum verification fails
> >>
> >> 4) read the page again (possibly reading a bit more new data)
> >>
> >> 5) the LSN did not change compared to the first read, yet the checksum
> >> still fails
> >
> > So, I agree with all of the above though I've found it to be extremely
> > rare to get a single read which you've managed to catch part-way through
> > a write, getting multiple of them over a period of time strikes me as
> > even more unlikely. Still, if we can come up with a solution to solve
> > all of this, great, but I'm not sure that I'm hearing one.

I don't recall claiming catching many such torn pages - I'm sure it's
> not very common in most workloads. But I suspect constructing workloads
> hitting them regularly is not very difficult either (something with a
> lot of churn in shared buffers should do the trick).

The question is if it’s possible to catch a torn page where the second half
is updated *before* the first half of the page in a read (and then in
subsequent reads having that state be maintained). I have some skepticism
that it’s really possible to happen in the first place but having an
interrupted system call be stalled across two more system calls just seems
terribly unlikely, and this is all based on the assumption that the kernel
might write the second half of a write before the first to the kernel cache
in the first place.

>>> Sure, because we don't care about it any longer- that page isn't
> >>> interesting because the WAL will replay over it. IIRC it actually goes
> >>> something like: check the checksum, if it failed then check if the LSN
> >>> is greater than the checkpoint (of the backup start..), if not, then
> >>> re-read, if the LSN is now newer than the checkpoint then skip, if the
> >>> LSN is the same then throw an error.
> >>
> >> Nope, we only verify the checksum if it's LSN precedes the checkpoint:
> >>
> >>
> https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454
> >
> > That seems like it's leaving something on the table, but, to be fair, we
> > know that all of those pages should be rewritten by WAL anyway so they
> > aren't all that interesting to us, particularly in the basebackup case.
> >
>
> Yep.
>
> >>> I actually tend to disagree with you that, for this purpose, it's
> >>> actually necessary to check against the checkpoint LSN- if the LSN
> >>> changed and everything is operating correctly then the new LSN must be
> >>> more recent than the last checkpoint location or things are broken
> >>> badly.
> >>
> >> I don't follow. Are you suggesting we don't need the checkpoint LSN?
> >>
> >> I'm pretty sure that's not the case. The thing is - the LSN may not
> >> change between the two reads, but that's not a guarantee the page was
> >> not torn. The example I posted earlier in this message illustrates that.
> >
> > I agree that there's some risk there, but it's certainly much less
> > likely.
> >
>
> Well. If we're going to report a checksum failure, we better be sure it
> actually is a broken page. I don't want users to start chasing bogus
> data corruption issues.

Yes, I definitely agree that we don’t want to mis-report checksum failures
if we can avoid it.

>>> Now, that said, I do think it's a good *idea* to check against the
> >>> checkpoint LSN (presuming this is for online checking of checksums- for
> >>> basebackup, we could just check against the backup-start LSN as
> anything
> >>> after that point will be rewritten by WAL anyway). The reason that I
> >>> think it's a good idea to check against the checkpoint LSN is that we'd
> >>> want to throw a big warning if the kernel is just feeding us random
> >>> garbage on reads and only finding a difference between two reads isn't
> >>> really doing any kind of validation, whereas checking against the
> >>> checkpoint-LSN would at least give us some idea that the value being
> >>> read isn't completely ridiculous.
> >>>
> >>> When it comes to if the pg_sleep() is necessary or not, I have to admit
> >>> to being unsure about that.. I could see how it might be but it seems
> a
> >>> bit surprising- I'd probably want to see exactly what the page was at
> >>> the time of the failure and at the time of the second (no-sleep)
> re-read
> >>> and then after a delay and convince myself that it was just an unlucky
> >>> case of being scheduled in twice to read that page before the process
> >>> writing it out got a chance to finish the write.
> >>
> >> I think the pg_sleep() is a pretty strong sign there's something broken.
> >> At the very least, it's likely to misbehave on machines with different
> >> timings, machines under memory and/or memory pressure, etc.
> >
> > If we assume that what you've outlined above is a serious enough issue
> > that we have to address it, and do so without a pg_sleep(), then I think
> > we have to bake into this a way for the process to check with PG as to
> > what the page's current LSN is, in shared buffers, because that's the
> > only place where we've got the locking required to ensure that we don't
> > end up with a read of a partially written page, and I'm really not
> > entirely convinced that we need to go to that level. It'd certainly add
> > a huge amount of additional complexity for what appears to be a quite
> > unlikely gain.
> >
> > I'll chat w/ David shortly about this again though and get his thoughts
> > on it. This is certainly an area we've spent time thinking about but
> > are obviously also open to finding a better solution.

Why not to simply look at the last checkpoint LSN and use that the same
> way basebackup does? AFAICS that should make the pg_sleep() unnecessary.

Use that to compare to what? The LSN in the first half of the page could
be from well before the checkpoint or even the backup started.

Thanks!

Stephen

>

Hi,

so, trying some intermediate summary here, sorry for (also) top-posting:

1. the basebackup checksum verification logic only checks pages not
changed since the checkpoint, which makes sense for the basebackup. 

2. However, it would be desirable to go further for pg_verify_checksums
and (re-)check all pages.

3. pg_verify_checksums should read the checkpoint LSN on startup and
compare the page LSN against it on re-read, and discard pages which have
checksum failures but are new. (Maybe it should read new checkpoint LSNs
as they come in during its runtime as well? See below).

4. The pg_sleep should go.

5. There seems to be no consensus on whether the number of skipped pages
should be summarized at the end.

Further comments:

Am Montag, den 17.09.2018, 19:19 +0200 schrieb Tomas Vondra:
> On 09/17/2018 07:11 PM, Stephen Frost wrote:
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> > > On 09/17/2018 06:42 PM, Stephen Frost wrote:
> > > Without the checkpoint that's not guaranteed, and simply re-reading the
> > > page and rechecking it vs. the first read does not help:
> > >
> > > 1) write the first 512B of the page (sector), which includes the LSN
> > >
> > > 2) read the whole page, which will be a mix [new 512B, ... old ... ]
> > >
> > > 3) the checksum verification fails
> > >
> > > 4) read the page again (possibly reading a bit more new data)
> > >
> > > 5) the LSN did not change compared to the first read, yet the checksum
> > > still fails
> >
> > So, I agree with all of the above though I've found it to be extremely
> > rare to get a single read which you've managed to catch part-way through
> > a write, getting multiple of them over a period of time strikes me as
> > even more unlikely. Still, if we can come up with a solution to solve
> > all of this, great, but I'm not sure that I'm hearing one.
>
> I don't recall claiming catching many such torn pages - I'm sure it's
> not very common in most workloads. But I suspect constructing workloads
> hitting them regularly is not very difficult either (something with a
> lot of churn in shared buffers should do the trick).
>
> > > > Sure, because we don't care about it any longer- that page isn't
> > > > interesting because the WAL will replay over it. IIRC it actually goes
> > > > something like: check the checksum, if it failed then check if the LSN
> > > > is greater than the checkpoint (of the backup start..), if not, then
> > > > re-read, if the LSN is now newer than the checkpoint then skip, if the
> > > > LSN is the same then throw an error.
> > >
> > > Nope, we only verify the checksum if it's LSN precedes the checkpoint:
> > >
> > > https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454
> >
> > That seems like it's leaving something on the table, but, to be fair, we
> > know that all of those pages should be rewritten by WAL anyway so they
> > aren't all that interesting to us, particularly in the basebackup case.
>
> Yep.

Right, see point 1 above.

> > > > I actually tend to disagree with you that, for this purpose, it's
> > > > actually necessary to check against the checkpoint LSN- if the LSN
> > > > changed and everything is operating correctly then the new LSN must be
> > > > more recent than the last checkpoint location or things are broken
> > > > badly.
> > >
> > > I don't follow. Are you suggesting we don't need the checkpoint LSN?
> > >
> > > I'm pretty sure that's not the case. The thing is - the LSN may not
> > > change between the two reads, but that's not a guarantee the page was
> > > not torn. The example I posted earlier in this message illustrates that.
> >
> > I agree that there's some risk there, but it's certainly much less
> > likely.
>
> Well. If we're going to report a checksum failure, we better be sure it
> actually is a broken page. I don't want users to start chasing bogus
> data corruption issues.

I agree.

> > > > Now, that said, I do think it's a good *idea* to check against the
> > > > checkpoint LSN (presuming this is for online checking of checksums- for
> > > > basebackup, we could just check against the backup-start LSN as anything
> > > > after that point will be rewritten by WAL anyway). The reason that I
> > > > think it's a good idea to check against the checkpoint LSN is that we'd
> > > > want to throw a big warning if the kernel is just feeding us random
> > > > garbage on reads and only finding a difference between two reads isn't
> > > > really doing any kind of validation, whereas checking against the
> > > > checkpoint-LSN would at least give us some idea that the value being
> > > > read isn't completely ridiculous.

Are you suggesting here that we always check against the current
checkpoint, or is checking against the checkpoint that we saw at startup
enough? I think re-reading pg_control all the time might be more
errorprone that what we could get from this, so I would prefer not to do
this.

> > > > When it comes to if the pg_sleep() is necessary or not, I have to admit
> > > > to being unsure about that.. I could see how it might be but it seems a
> > > > bit surprising- I'd probably want to see exactly what the page was at
> > > > the time of the failure and at the time of the second (no-sleep) re-read
> > > > and then after a delay and convince myself that it was just an unlucky
> > > > case of being scheduled in twice to read that page before the process
> > > > writing it out got a chance to finish the write.
> > >
> > > I think the pg_sleep() is a pretty strong sign there's something broken.
> > > At the very least, it's likely to misbehave on machines with different
> > > timings, machines under memory and/or memory pressure, etc.

I swapped out the pg_sleep earlier today for the check-against-
checkpoint-LSN-on-reread, and that seems to work just as fine, at least
in the tests I ran.

> > If we assume that what you've outlined above is a serious enough issue
> > that we have to address it, and do so without a pg_sleep(), then I think
> > we have to bake into this a way for the process to check with PG as to
> > what the page's current LSN is, in shared buffers, because that's the
> > only place where we've got the locking required to ensure that we don't
> > end up with a read of a partially written page, and I'm really not
> > entirely convinced that we need to go to that level. It'd certainly add
> > a huge amount of additional complexity for what appears to be a quite
> > unlikely gain.
> >
> > I'll chat w/ David shortly about this again though and get his thoughts
> > on it. This is certainly an area we've spent time thinking about but
> > are obviously also open to finding a better solution.
>
> Why not to simply look at the last checkpoint LSN and use that the same
> way basebackup does? AFAICS that should make the pg_sleep() unnecessary.

Right.

Michael

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

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

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

Greetings,

On Mon, Sep 17, 2018 at 13:38 Michael Banck <michael(dot)banck(at)credativ(dot)de>
wrote:

> so, trying some intermediate summary here, sorry for (also) top-posting:
>
> 1. the basebackup checksum verification logic only checks pages not
> changed since the checkpoint, which makes sense for the basebackup.

Right. I’m tending towards the idea that this also be adopted for
pg_verify_checksums.

2. However, it would be desirable to go further for pg_verify_checksums
> and (re-)check all pages.

Maybe. I’m not entirely convinced that it’s all that useful.

3. pg_verify_checksums should read the checkpoint LSN on startup and
> compare the page LSN against it on re-read, and discard pages which have
> checksum failures but are new. (Maybe it should read new checkpoint LSNs
> as they come in during its runtime as well? See below).

I’m not sure that we really need to but I’m not against it either- but in
that case you’re definitely going to see checksum failures on torn pages.

4. The pg_sleep should go.

I know that pgbackrest does not have a sleep currently and we’ve not yet
seen or been able to reproduce this case where, on a reread, we still see
an older LSN, but we check the LSN first also. If it’s possible that the
LSN still hasn’t changed on the reread then maybe we do need to have a
sleep to force ourselves off CPU to allow the other process to finish
writing, or maybe finish the file and come back around to these pages
later, but we have yet to see this behavior in the wild anywhere, nor have
we been able to reproduce it.

5. There seems to be no consensus on whether the number of skipped pages
> should be summarized at the end.

I agree with printing the number of skipped pages, that does seem like a
nice to have. I don’t know that actually printing the pages themselves is
all that useful though.

Further comments:
>
> Am Montag, den 17.09.2018, 19:19 +0200 schrieb Tomas Vondra:
> > On 09/17/2018 07:11 PM, Stephen Frost wrote:
> > > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> > > > On 09/17/2018 06:42 PM, Stephen Frost wrote:
> > > > Without the checkpoint that's not guaranteed, and simply re-reading
> the
> > > > page and rechecking it vs. the first read does not help:
> > > >
> > > > 1) write the first 512B of the page (sector), which includes the LSN
> > > >
> > > > 2) read the whole page, which will be a mix [new 512B, ... old ... ]
> > > >
> > > > 3) the checksum verification fails
> > > >
> > > > 4) read the page again (possibly reading a bit more new data)
> > > >
> > > > 5) the LSN did not change compared to the first read, yet the
> checksum
> > > > still fails
> > >
> > > So, I agree with all of the above though I've found it to be extremely
> > > rare to get a single read which you've managed to catch part-way
> through
> > > a write, getting multiple of them over a period of time strikes me as
> > > even more unlikely. Still, if we can come up with a solution to solve
> > > all of this, great, but I'm not sure that I'm hearing one.
> >
> > I don't recall claiming catching many such torn pages - I'm sure it's
> > not very common in most workloads. But I suspect constructing workloads
> > hitting them regularly is not very difficult either (something with a
> > lot of churn in shared buffers should do the trick).
> >
> > > > > Sure, because we don't care about it any longer- that page isn't
> > > > > interesting because the WAL will replay over it. IIRC it actually
> goes
> > > > > something like: check the checksum, if it failed then check if the
> LSN
> > > > > is greater than the checkpoint (of the backup start..), if not,
> then
> > > > > re-read, if the LSN is now newer than the checkpoint then skip, if
> the
> > > > > LSN is the same then throw an error.
> > > >
> > > > Nope, we only verify the checksum if it's LSN precedes the
> checkpoint:
> > > >
> > > >
> https://github.com/postgres/postgres/blob/master/src/backend/replication/basebackup.c#L1454
> > >
> > > That seems like it's leaving something on the table, but, to be fair,
> we
> > > know that all of those pages should be rewritten by WAL anyway so they
> > > aren't all that interesting to us, particularly in the basebackup case.
> >
> > Yep.
>
> Right, see point 1 above.
>
> > > > > I actually tend to disagree with you that, for this purpose, it's
> > > > > actually necessary to check against the checkpoint LSN- if the LSN
> > > > > changed and everything is operating correctly then the new LSN
> must be
> > > > > more recent than the last checkpoint location or things are broken
> > > > > badly.
> > > >
> > > > I don't follow. Are you suggesting we don't need the checkpoint LSN?
> > > >
> > > > I'm pretty sure that's not the case. The thing is - the LSN may not
> > > > change between the two reads, but that's not a guarantee the page was
> > > > not torn. The example I posted earlier in this message illustrates
> that.
> > >
> > > I agree that there's some risk there, but it's certainly much less
> > > likely.
> >
> > Well. If we're going to report a checksum failure, we better be sure it
> > actually is a broken page. I don't want users to start chasing bogus
> > data corruption issues.
>
> I agree.
>
> > > > > Now, that said, I do think it's a good *idea* to check against the
> > > > > checkpoint LSN (presuming this is for online checking of
> checksums- for
> > > > > basebackup, we could just check against the backup-start LSN as
> anything
> > > > > after that point will be rewritten by WAL anyway). The reason
> that I
> > > > > think it's a good idea to check against the checkpoint LSN is that
> we'd
> > > > > want to throw a big warning if the kernel is just feeding us random
> > > > > garbage on reads and only finding a difference between two reads
> isn't
> > > > > really doing any kind of validation, whereas checking against the
> > > > > checkpoint-LSN would at least give us some idea that the value
> being
> > > > > read isn't completely ridiculous.
>
> Are you suggesting here that we always check against the current
> checkpoint, or is checking against the checkpoint that we saw at startup
> enough? I think re-reading pg_control all the time might be more
> errorprone that what we could get from this, so I would prefer not to do
> this.

I don’t follow why rereading pg_control would be error-prone. That said, I
don’t have a particularly strong opinion either way on this.

> > > > When it comes to if the pg_sleep() is necessary or not, I have to
> admit
> > > > > to being unsure about that.. I could see how it might be but it
> seems a
> > > > > bit surprising- I'd probably want to see exactly what the page was
> at
> > > > > the time of the failure and at the time of the second (no-sleep)
> re-read
> > > > > and then after a delay and convince myself that it was just an
> unlucky
> > > > > case of being scheduled in twice to read that page before the
> process
> > > > > writing it out got a chance to finish the write.
> > > >
> > > > I think the pg_sleep() is a pretty strong sign there's something
> broken.
> > > > At the very least, it's likely to misbehave on machines with
> different
> > > > timings, machines under memory and/or memory pressure, etc.
>
> I swapped out the pg_sleep earlier today for the check-against-
> checkpoint-LSN-on-reread, and that seems to work just as fine, at least
> in the tests I ran.

Ok, this sounds like you were probably seeing normal forward torn pages,
and we have certainly seen that before.

> > If we assume that what you've outlined above is a serious enough issue
> > > that we have to address it, and do so without a pg_sleep(), then I
> think
> > > we have to bake into this a way for the process to check with PG as to
> > > what the page's current LSN is, in shared buffers, because that's the
> > > only place where we've got the locking required to ensure that we don't
> > > end up with a read of a partially written page, and I'm really not
> > > entirely convinced that we need to go to that level. It'd certainly
> add
> > > a huge amount of additional complexity for what appears to be a quite
> > > unlikely gain.
> > >
> > > I'll chat w/ David shortly about this again though and get his thoughts
> > > on it. This is certainly an area we've spent time thinking about but
> > > are obviously also open to finding a better solution.
> >
> > Why not to simply look at the last checkpoint LSN and use that the same
> > way basebackup does? AFAICS that should make the pg_sleep() unnecessary.
>
> Right.

This is fine if you know the kernel will always write the first page first,
or you accept that a reread of a page which isn’t valid will always result
in seeing a completely updated page.

We’ve made the assumption that a reread on a failure where the LSN on the
first read was older than the backup-start LSN will give us an updated
first-half of the page which we then check the LSN, of, but we have yet to
prove that this is actually possible.

Thanks!

Stephen

On 09/17/2018 07:35 PM, Stephen Frost wrote:
> Greetings,
>
> On Mon, Sep 17, 2018 at 13:20 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>> wrote:
>
> On 09/17/2018 07:11 PM, Stephen Frost wrote:
> > Greetings,
> >
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com
> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>) wrote:
> >> On 09/17/2018 06:42 PM, Stephen Frost wrote:
> >>> Ok, good, though I'm not sure what you mean by 'eliminates the
> >>> consistency guarantees provided by the checkpoint'.  The point
> is that
> >>> the page will be in the WAL and the WAL will be replayed during the
> >>> restore of the backup.
> >>
> >> The checkpoint guarantees that the whole page was written and
> flushed to
> >> disk with an LSN before the ckeckpoint LSN. So when you read a
> page with
> >> that LSN, you know the whole write already completed and a read won't
> >> return data from before the LSN.
> >
> > Well, you know that the first part was written out at some prior
> point,
> > but you could end up reading the first part of a page with an
> older LSN
> > while also reading the second part with new data.
>
>
>
> Doesn't the checkpoint fsync pretty much guarantee this can't happen?
>
>
> How? Either it’s possible for the latter half of a page to be updated
> before the first half (where the LSN lives), or it isn’t. If it’s
> possible then that LSN could be ancient and it wouldn’t matter. 
>

I'm not sure I understand what you're saying here.

It is not about the latter page to be updated before the first half. I
don't think that's quite possible, because write() into page cache does
in fact write the data sequentially.

The problem is that the write is not atomic, and AFAIK it happens in
sectors (which are either 512B or 4K these days). And it may arbitrarily
interleave with reads.

So you may do write(8k), but it actually happens in 512B chunks and a
concurrent read may observe some mix of those.

But the trick is that if the read sees the effect of the write somewhere
in the middle of the page, the next read is guaranteed to see all the
preceding new data.

Without the checkpoint we risk seeing the same write() both in read and
re-read, just in a different stage - so the LSN would not change, making
the check futile.

But by waiting for the checkpoint we know that the original write is no
longer in progress, so if we saw a partial write we're guaranteed to see
a new LSN on re-read.

This is what I mean by the checkpoint / fsync guarantee.

> >> Without the checkpoint that's not guaranteed, and simply
> re-reading the
> >> page and rechecking it vs. the first read does not help:
> >>
> >> 1) write the first 512B of the page (sector), which includes the LSN
> >>
> >> 2) read the whole page, which will be a mix [new 512B, ... old ... ]
> >>
> >> 3) the checksum verification fails
> >>
> >> 4) read the page again (possibly reading a bit more new data)
> >>
> >> 5) the LSN did not change compared to the first read, yet the
> checksum
> >> still fails
> >
> > So, I agree with all of the above though I've found it to be extremely
> > rare to get a single read which you've managed to catch part-way
> through
> > a write, getting multiple of them over a period of time strikes me as
> > even more unlikely.  Still, if we can come up with a solution to solve
> > all of this, great, but I'm not sure that I'm hearing one.
>
>
> I don't recall claiming catching many such torn pages - I'm sure it's
> not very common in most workloads. But I suspect constructing workloads
> hitting them regularly is not very difficult either (something with a
> lot of churn in shared buffers should do the trick).
>
>
> The question is if it’s possible to catch a torn page where the second
> half is updated *before* the first half of the page in a read (and then
> in subsequent reads having that state be maintained).  I have some
> skepticism that it’s really possible to happen in the first place but
> having an interrupted system call be stalled across two more system
> calls just seems terribly unlikely, and this is all based on the
> assumption that the kernel might write the second half of a write before
> the first to the kernel cache in the first place.
>

Yes, if that was possible, the explanation about the checkpoint fsync
guarantee would be bogus, obviously.

I've spent quite a bit of time looking into how write() is handled, and
I believe seeing only the second half is not possible. You may observe a
page torn in various ways (not necessarily in half), e.g.

[old,new,old]

but then the re-read you should be guaranteed to see new data up until
the last "new" chunk:

[new,new,old]

At least that's my understanding. I failed to deduce what POSIX says
about this, or how it behaves on various OS/filesystems.

The one thing I've done was writing a simple stress test that writes a
single 8kB page in a loop, reads it concurrently and checks the
behavior. And it seems consistent with my understanding.

>
> >>> Now, that said, I do think it's a good *idea* to check against the
> >>> checkpoint LSN (presuming this is for online checking of
> checksums- for
> >>> basebackup, we could just check against the backup-start LSN as
> anything
> >>> after that point will be rewritten by WAL anyway).  The reason
> that I
> >>> think it's a good idea to check against the checkpoint LSN is
> that we'd
> >>> want to throw a big warning if the kernel is just feeding us random
> >>> garbage on reads and only finding a difference between two reads
> isn't
> >>> really doing any kind of validation, whereas checking against the
> >>> checkpoint-LSN would at least give us some idea that the value being
> >>> read isn't completely ridiculous.
> >>>
> >>> When it comes to if the pg_sleep() is necessary or not, I have
> to admit
> >>> to being unsure about that..  I could see how it might be but it
> seems a
> >>> bit surprising- I'd probably want to see exactly what the page
> was at
> >>> the time of the failure and at the time of the second (no-sleep)
> re-read
> >>> and then after a delay and convince myself that it was just an
> unlucky
> >>> case of being scheduled in twice to read that page before the
> process
> >>> writing it out got a chance to finish the write.
> >>
> >> I think the pg_sleep() is a pretty strong sign there's something
> broken.
> >> At the very least, it's likely to misbehave on machines with
> different
> >> timings, machines under memory and/or memory pressure, etc.
> >
> > If we assume that what you've outlined above is a serious enough issue
> > that we have to address it, and do so without a pg_sleep(), then I
> think
> > we have to bake into this a way for the process to check with PG as to
> > what the page's current LSN is, in shared buffers, because that's the
> > only place where we've got the locking required to ensure that we
> don't
> > end up with a read of a partially written page, and I'm really not
> > entirely convinced that we need to go to that level.  It'd
> certainly add
> > a huge amount of additional complexity for what appears to be a quite
> > unlikely gain.
> >
> > I'll chat w/ David shortly about this again though and get his
> thoughts
> > on it.  This is certainly an area we've spent time thinking about but
> > are obviously also open to finding a better solution.
>
>
> Why not to simply look at the last checkpoint LSN and use that the same
> way basebackup does? AFAICS that should make the pg_sleep() unnecessary.
>
>
> Use that to compare to what?  The LSN in the first half of the page
> could be from well before the checkpoint or even the backup started.
>

Not sure I follow. If the LSN in the page header is old, and the
checksum check failed, then on re-read we either find a new LSN (in
which case we skip the page) or consider this to be a checksum failure.

regards

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

Greetings,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 09/17/2018 07:35 PM, Stephen Frost wrote:
> > On Mon, Sep 17, 2018 at 13:20 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
> > <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>> wrote:
> > Doesn't the checkpoint fsync pretty much guarantee this can't happen?
> >
> > How? Either it’s possible for the latter half of a page to be updated
> > before the first half (where the LSN lives), or it isn’t. If it’s
> > possible then that LSN could be ancient and it wouldn’t matter. 
>
> I'm not sure I understand what you're saying here.
>
> It is not about the latter page to be updated before the first half. I
> don't think that's quite possible, because write() into page cache does
> in fact write the data sequentially.

Well, maybe 'updated before' wasn't quite the right way to talk about
it, but consider if a read(8K) gets only half-way through the copy
before having to go do something else and by the time it gets back, a
write has come in and rewritten the page, such that the read(8K)
returns half-old and half-new data.

> The problem is that the write is not atomic, and AFAIK it happens in
> sectors (which are either 512B or 4K these days). And it may arbitrarily
> interleave with reads.

Yes, of course the write isn't atomic, that's clear.

> So you may do write(8k), but it actually happens in 512B chunks and a
> concurrent read may observe some mix of those.

Right, I'm not sure that we really need to worry about sub-4K writes
though I suppose they're technically possible, but it doesn't much
matter in this case since the LSN is early on in the page, of course.

> But the trick is that if the read sees the effect of the write somewhere
> in the middle of the page, the next read is guaranteed to see all the
> preceding new data.

If that's guaranteed then we can just check the LSN and be done.

> Without the checkpoint we risk seeing the same write() both in read and
> re-read, just in a different stage - so the LSN would not change, making
> the check futile.

This is the part that isn't making much sense to me. If we are
guaranteed that writes into the kernel cache are always in order and
always at least 512B in size, then if we check the LSN first and
discover it's "old", and then read the rest of the page and calculate
the checksum, discover it's a bad checksum, and then go back and re-read
the page then we *must* see that the LSN has changed OR conclude that
the checksum is invalidated.

The reason this can happen in the first place is that our 8K read might
only get half-way done before getting scheduled off and a 8K write
happened on the page before our read(8K) gets back to finishing the
read, but if what you're saying is true, then we can't ever have a case
where such a thing would happen and a re-read would still see the "old"
LSN.

If we check the LSN first and discover it's "new" (as in, more recent
than our last checkpoint, or the checkpoint where the backup started)
then, sure, there's going to be a risk that the page is currently being
written right that moment and isn't yet completely valid.

The problem that we aren't solving for is if, somehow, we do a read(8K)
and get the first half/second half mixup and then on a subsequent
read(8K) we see that *again*, implying that somehow the kernel's copy
has the latter-half of the page updated consistently but not the first
half. That's a problem that I haven't got a solution to today. I'd
love to have a guarantee that it's not possible- we've certainly never
seen it but it's been a concern and I thought Michael was suggesting
he'd seen that, but it sounds like there wasn't a check on the LSN in
the first read, in which case it could have just been a 'regular' torn
page case.

> But by waiting for the checkpoint we know that the original write is no
> longer in progress, so if we saw a partial write we're guaranteed to see
> a new LSN on re-read.
>
> This is what I mean by the checkpoint / fsync guarantee.

I don't think any of this really has anythign to do with either fsync
being called or with the actual checkpointing process (except to the
extent that the checkpointer is the thing doing the writing, and that we
should be checking the LSN against the LSN of the last checkpoint when
we started, or against the start of the backup LSN if we're talking
about doing a backup).

> > The question is if it’s possible to catch a torn page where the second
> > half is updated *before* the first half of the page in a read (and then
> > in subsequent reads having that state be maintained).  I have some
> > skepticism that it’s really possible to happen in the first place but
> > having an interrupted system call be stalled across two more system
> > calls just seems terribly unlikely, and this is all based on the
> > assumption that the kernel might write the second half of a write before
> > the first to the kernel cache in the first place.
>
> Yes, if that was possible, the explanation about the checkpoint fsync
> guarantee would be bogus, obviously.
>
> I've spent quite a bit of time looking into how write() is handled, and
> I believe seeing only the second half is not possible. You may observe a
> page torn in various ways (not necessarily in half), e.g.
>
> [old,new,old]
>
> but then the re-read you should be guaranteed to see new data up until
> the last "new" chunk:
>
> [new,new,old]
>
> At least that's my understanding. I failed to deduce what POSIX says
> about this, or how it behaves on various OS/filesystems.
>
> The one thing I've done was writing a simple stress test that writes a
> single 8kB page in a loop, reads it concurrently and checks the
> behavior. And it seems consistent with my understanding.

Good.

> > Use that to compare to what?  The LSN in the first half of the page
> > could be from well before the checkpoint or even the backup started.
>
> Not sure I follow. If the LSN in the page header is old, and the
> checksum check failed, then on re-read we either find a new LSN (in
> which case we skip the page) or consider this to be a checksum failure.

Right, I'm in agreement with doing that and it's what is done in
pgbasebackup and pgBackRest.

Thanks!

Stephen

On 09/18/2018 12:01 AM, Stephen Frost wrote:
> Greetings,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
>> On 09/17/2018 07:35 PM, Stephen Frost wrote:
>>> On Mon, Sep 17, 2018 at 13:20 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
>>> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>> wrote:
>>> Doesn't the checkpoint fsync pretty much guarantee this can't happen?
>>>
>>> How? Either it’s possible for the latter half of a page to be updated
>>> before the first half (where the LSN lives), or it isn’t. If it’s
>>> possible then that LSN could be ancient and it wouldn’t matter. 
>>
>> I'm not sure I understand what you're saying here.
>>
>> It is not about the latter page to be updated before the first half. I
>> don't think that's quite possible, because write() into page cache does
>> in fact write the data sequentially.
>
> Well, maybe 'updated before' wasn't quite the right way to talk about
> it, but consider if a read(8K) gets only half-way through the copy
> before having to go do something else and by the time it gets back, a
> write has come in and rewritten the page, such that the read(8K)
> returns half-old and half-new data.
>
>> The problem is that the write is not atomic, and AFAIK it happens in
>> sectors (which are either 512B or 4K these days). And it may arbitrarily
>> interleave with reads.
>
> Yes, of course the write isn't atomic, that's clear.
>
>> So you may do write(8k), but it actually happens in 512B chunks and a
>> concurrent read may observe some mix of those.
>
> Right, I'm not sure that we really need to worry about sub-4K writes
> though I suppose they're technically possible, but it doesn't much
> matter in this case since the LSN is early on in the page, of course.
>
>> But the trick is that if the read sees the effect of the write somewhere
>> in the middle of the page, the next read is guaranteed to see all the
>> preceding new data.
>
> If that's guaranteed then we can just check the LSN and be done.
>

What do you mean by "check the LSN"? Compare it to LSN from the first
read? You don't know if the first read already saw the new LSN or not
(see the next example).

>> Without the checkpoint we risk seeing the same write() both in read and
>> re-read, just in a different stage - so the LSN would not change, making
>> the check futile.
>
> This is the part that isn't making much sense to me. If we are
> guaranteed that writes into the kernel cache are always in order and
> always at least 512B in size, then if we check the LSN first and
> discover it's "old", and then read the rest of the page and calculate
> the checksum, discover it's a bad checksum, and then go back and re-read
> the page then we *must* see that the LSN has changed OR conclude that
> the checksum is invalidated.
>

Even if the writes are in order and in 512B chunks, you don't know how
they are interleaved with the reads.

Let's assume we're doing a write(), which splits the 8kB page into 512B
chunks. A concurrent read may observe a random mix of old and new data,
depending on timing.

So let's say a read sees the first 2kB of data like this:

[new, new, new, old, new, old, new, old]

OK, the page is obviously torn, checksum fails, and we try reading it
again. We should see new data at least until the last 'new' chunk in the
first read, so let's say we got this:

[new, new, new, new, new, new, new, old]

Obviously, this page is also torn (there are old data at the end), but
we've read the new data in both cases, which includes the LSN. So the
LSN is the same in both cases, and your detection fails.

Comparing the page LSN to the last checkpoint LSN solves this, because
if the LSN is older than the checkpoint LSN, that write must have been
completed by now, and so we're not in danger of seeing only incomplete
effects of it. And newer write will update the LSN.

> The reason this can happen in the first place is that our 8K read might
> only get half-way done before getting scheduled off and a 8K write
> happened on the page before our read(8K) gets back to finishing the
> read, but if what you're saying is true, then we can't ever have a case
> where such a thing would happen and a re-read would still see the "old"
> LSN.
>
> If we check the LSN first and discover it's "new" (as in, more recent
> than our last checkpoint, or the checkpoint where the backup started)
> then, sure, there's going to be a risk that the page is currently being
> written right that moment and isn't yet completely valid.
>

Right.

> The problem that we aren't solving for is if, somehow, we do a read(8K)
> and get the first half/second half mixup and then on a subsequent
> read(8K) we see that *again*, implying that somehow the kernel's copy
> has the latter-half of the page updated consistently but not the first
> half. That's a problem that I haven't got a solution to today. I'd
> love to have a guarantee that it's not possible- we've certainly never
> seen it but it's been a concern and I thought Michael was suggesting
> he'd seen that, but it sounds like there wasn't a check on the LSN in
> the first read, in which case it could have just been a 'regular' torn
> page case.
>

Well, yeah. If that would be possible, we'd be in serious trouble. I've
done quite a bit of experimentation with concurrent reads and writes and
I have not observed such behavior. Of course, that's hardly a proof it
can't happen, and it wouldn't be the first surprise with respect to
kernel I/O this year ...

>> But by waiting for the checkpoint we know that the original write is no
>> longer in progress, so if we saw a partial write we're guaranteed to see
>> a new LSN on re-read.
>>
>> This is what I mean by the checkpoint / fsync guarantee.
>
> I don't think any of this really has anythign to do with either fsync
> being called or with the actual checkpointing process (except to the
> extent that the checkpointer is the thing doing the writing, and that we
> should be checking the LSN against the LSN of the last checkpoint when
> we started, or against the start of the backup LSN if we're talking
> about doing a backup).
>

You're right it's not about the fsync, sorry for the confusion. My point
is that using the checkpoint LSN gives us a guarantee that write is no
longer in progress, and so we can't see a page torn because of it. And
if we see a partial write due to a new write, it's guaranteed to update
the page LSN (and we'll notice it).

>>> The question is if it’s possible to catch a torn page where the second
>>> half is updated *before* the first half of the page in a read (and then
>>> in subsequent reads having that state be maintained).  I have some
>>> skepticism that it’s really possible to happen in the first place but
>>> having an interrupted system call be stalled across two more system
>>> calls just seems terribly unlikely, and this is all based on the
>>> assumption that the kernel might write the second half of a write before
>>> the first to the kernel cache in the first place.
>>
>> Yes, if that was possible, the explanation about the checkpoint fsync
>> guarantee would be bogus, obviously.
>>
>> I've spent quite a bit of time looking into how write() is handled, and
>> I believe seeing only the second half is not possible. You may observe a
>> page torn in various ways (not necessarily in half), e.g.
>>
>> [old,new,old]
>>
>> but then the re-read you should be guaranteed to see new data up until
>> the last "new" chunk:
>>
>> [new,new,old]
>>
>> At least that's my understanding. I failed to deduce what POSIX says
>> about this, or how it behaves on various OS/filesystems.
>>
>> The one thing I've done was writing a simple stress test that writes a
>> single 8kB page in a loop, reads it concurrently and checks the
>> behavior. And it seems consistent with my understanding.
>
> Good.
>
>>> Use that to compare to what?  The LSN in the first half of the page
>>> could be from well before the checkpoint or even the backup started.
>>
>> Not sure I follow. If the LSN in the page header is old, and the
>> checksum check failed, then on re-read we either find a new LSN (in
>> which case we skip the page) or consider this to be a checksum failure.
>
> Right, I'm in agreement with doing that and it's what is done in
> pgbasebackup and pgBackRest.
>

OK. All I'm saying is pg_verify_checksums should probably do the same
thing, i.e. grab checkpoint LSN and roll with that.

regards

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

Greetings,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 09/18/2018 12:01 AM, Stephen Frost wrote:
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> >> On 09/17/2018 07:35 PM, Stephen Frost wrote:
> >> But the trick is that if the read sees the effect of the write somewhere
> >> in the middle of the page, the next read is guaranteed to see all the
> >> preceding new data.
> >
> > If that's guaranteed then we can just check the LSN and be done.
>
> What do you mean by "check the LSN"? Compare it to LSN from the first
> read? You don't know if the first read already saw the new LSN or not
> (see the next example).

Hmm, ok, I can see your point there. I've been going back and forth
between checking against what the prior LSN was on the page and checking
it against an independent source (like the last checkpoint's LSN), but..

[...]

> Comparing the page LSN to the last checkpoint LSN solves this, because
> if the LSN is older than the checkpoint LSN, that write must have been
> completed by now, and so we're not in danger of seeing only incomplete
> effects of it. And newer write will update the LSN.

Yeah, that makes sense- we need to be looking at something which only
gets updated once the write has actually completed, and the last
checkpoint's LSN gives us that guarantee.

> > The problem that we aren't solving for is if, somehow, we do a read(8K)
> > and get the first half/second half mixup and then on a subsequent
> > read(8K) we see that *again*, implying that somehow the kernel's copy
> > has the latter-half of the page updated consistently but not the first
> > half. That's a problem that I haven't got a solution to today. I'd
> > love to have a guarantee that it's not possible- we've certainly never
> > seen it but it's been a concern and I thought Michael was suggesting
> > he'd seen that, but it sounds like there wasn't a check on the LSN in
> > the first read, in which case it could have just been a 'regular' torn
> > page case.
>
> Well, yeah. If that would be possible, we'd be in serious trouble. I've
> done quite a bit of experimentation with concurrent reads and writes and
> I have not observed such behavior. Of course, that's hardly a proof it
> can't happen, and it wouldn't be the first surprise with respect to
> kernel I/O this year ...

I'm glad to hear that you've done a lot of experimentation in this area
and haven't seen such strange behavior happen- we've got quite a few
people running pgBackRest with checksum-checking and haven't seen it
either, but it's always been a bit of a concern.

> You're right it's not about the fsync, sorry for the confusion. My point
> is that using the checkpoint LSN gives us a guarantee that write is no
> longer in progress, and so we can't see a page torn because of it. And
> if we see a partial write due to a new write, it's guaranteed to update
> the page LSN (and we'll notice it).

Right, no worries about the confusion, I hadn't been fully thinking
through the LSN bit either and that what we really need is some external
confirmation of a write having *completed* (not just started) and that
makes a definite difference.

> > Right, I'm in agreement with doing that and it's what is done in
> > pgbasebackup and pgBackRest.
>
> OK. All I'm saying is pg_verify_checksums should probably do the same
> thing, i.e. grab checkpoint LSN and roll with that.

Agreed.

Thanks!

Stephen

Hi,

Am Montag, den 17.09.2018, 14:09 -0400 schrieb Stephen Frost:
> > 5. There seems to be no consensus on whether the number of skipped pages
> > should be summarized at the end.
>
> I agree with printing the number of skipped pages, that does seem like
> a nice to have.  I don’t know that actually printing the pages
> themselves is all that useful though. 

Oh ok - I never intended to print out the block numbers themselves, just
the final number of skipped blocks in the summary. So I guess that's
fine and I will add that in my branch.

Michael

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

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

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

Hi.

Am Montag, den 17.09.2018, 20:45 -0400 schrieb Stephen Frost:
> > You're right it's not about the fsync, sorry for the confusion. My point
> > is that using the checkpoint LSN gives us a guarantee that write is no
> > longer in progress, and so we can't see a page torn because of it. And
> > if we see a partial write due to a new write, it's guaranteed to update
> > the page LSN (and we'll notice it).
>
> Right, no worries about the confusion, I hadn't been fully thinking
> through the LSN bit either and that what we really need is some external
> confirmation of a write having *completed* (not just started) and that
> makes a definite difference.
>
> > > Right, I'm in agreement with doing that and it's what is done in
> > > pgbasebackup and pgBackRest.
> >
> > OK. All I'm saying is pg_verify_checksums should probably do the same
> > thing, i.e. grab checkpoint LSN and roll with that.
>
> Agreed.

I've attached the patch I added to my branch to swap out the pg_sleep()
with a check against the checkpoint LSN on a recheck verification
failure.

Let me know if there are still issues with it. I'll send a new patch for
the whole online verification feature in a bit.

Michael

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

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

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

Hi,

please find attached version 2 of the patch.

Am Donnerstag, den 26.07.2018, 13:59 +0200 schrieb Michael Banck:
> I've now forward-ported this change to pg_verify_checksums, in order to
> make this application useful for online clusters, see attached patch.
>
> I've tested this in a tight loop (while true; do pg_verify_checksums -D
> data1 -d > /dev/null || /bin/true; done)[2] while doing "while true; do
> createdb pgbench; pgbench -i -s 10 pgbench > /dev/null; dropdb pgbench;
> done", which I already used to develop the original code in the fork and
> which brought up a few bugs.
>
> I got one checksums verification failure this way, all others were
> caught by the recheck (I've introduced a 500ms delay for the first ten
> failures) like this:
>
> > pg_verify_checksums: checksum verification failed on first attempt in
> > file "data1/base/16837/16850", block 7770: calculated checksum 785 but
> > expected 5063
> > pg_verify_checksums: block 7770 in file "data1/base/16837/16850"
> > verified ok on recheck

I have now changed this from the pg_sleep() to a check against the
checkpoint LSN as discussed upthread.

> However, I am also seeing sporadic (maybe 0.5 times per pgbench run)
> failures like this:
>
> > pg_verify_checksums: short read of block 2644 in file
> > "data1/base/16637/16650", got only 4096 bytes
>
> This is not strictly a verification failure, should we do anything about
> this? In my fork, I am also rechecking on this[3] (and I am happy to
> extend the patch that way), but that makes the code and the patch more
> complicated and I wanted to check the general opinion on this case
> first.

I have added a retry for this as well now, without a pg_sleep() as well.
This catches around 80% of the half-reads, but a few slip through. At
that point we bail out with exit(1), and the user can try again, which I
think is fine? 

Alternatively, we could just skip to the next file then and don't make
it count as a checksum failure.

Other changes from V1:

1. Rebased to 422952ee
2. Ignore ENOENT failure during file open and skip to next file
3. Mention total number of skipped blocks during the summary at the end
of the run
4. Skip files starting with pg_internal.init*

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> please find attached version 2 of the patch.
>
> Am Donnerstag, den 26.07.2018, 13:59 +0200 schrieb Michael Banck:
> > I've now forward-ported this change to pg_verify_checksums, in order to
> > make this application useful for online clusters, see attached patch.
> >
> > I've tested this in a tight loop (while true; do pg_verify_checksums -D
> > data1 -d > /dev/null || /bin/true; done)[2] while doing "while true; do
> > createdb pgbench; pgbench -i -s 10 pgbench > /dev/null; dropdb pgbench;
> > done", which I already used to develop the original code in the fork and
> > which brought up a few bugs.
> >
> > I got one checksums verification failure this way, all others were
> > caught by the recheck (I've introduced a 500ms delay for the first ten
> > failures) like this:
> >
> > > pg_verify_checksums: checksum verification failed on first attempt in
> > > file "data1/base/16837/16850", block 7770: calculated checksum 785 but
> > > expected 5063
> > > pg_verify_checksums: block 7770 in file "data1/base/16837/16850"
> > > verified ok on recheck
>
> I have now changed this from the pg_sleep() to a check against the
> checkpoint LSN as discussed upthread.

Ok.

> > However, I am also seeing sporadic (maybe 0.5 times per pgbench run)
> > failures like this:
> >
> > > pg_verify_checksums: short read of block 2644 in file
> > > "data1/base/16637/16650", got only 4096 bytes
> >
> > This is not strictly a verification failure, should we do anything about
> > this? In my fork, I am also rechecking on this[3] (and I am happy to
> > extend the patch that way), but that makes the code and the patch more
> > complicated and I wanted to check the general opinion on this case
> > first.
>
> I have added a retry for this as well now, without a pg_sleep() as well.

> This catches around 80% of the half-reads, but a few slip through. At
> that point we bail out with exit(1), and the user can try again, which I
> think is fine? 

No, this is perfectly normal behavior, as is having completely blank
pages, now that I think about it. If we get a short read then I'd say
we simply check that we got an EOF and, in that case, we just move on.

> Alternatively, we could just skip to the next file then and don't make
> it count as a checksum failure.

No, I wouldn't count it as a checksum failure. We could possibly count
it towards the skipped pages, though I'm even on the fence about that.

Thanks!

Stephen

On 9/18/18 11:45 AM, Stephen Frost wrote:
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:

>> I have added a retry for this as well now, without a pg_sleep() as well.
>
>> This catches around 80% of the half-reads, but a few slip through. At
>> that point we bail out with exit(1), and the user can try again, which I
>> think is fine? 
>
> No, this is perfectly normal behavior, as is having completely blank
> pages, now that I think about it. If we get a short read then I'd say
> we simply check that we got an EOF and, in that case, we just move on.
>
>> Alternatively, we could just skip to the next file then and don't make
>> it count as a checksum failure.
>
> No, I wouldn't count it as a checksum failure. We could possibly count
> it towards the skipped pages, though I'm even on the fence about that.

+1 for it not being a failure. Personally I'd count it as a skipped
page, since we know the page exists but it can't be verified.

The other option is to wait for the page to stabilize, which doesn't
seem like it would take very long in most cases -- unless you are doing
this test from another host with shared storage. Then I would expect to
see all kinds of interesting torn pages after the last checkpoint.

Regards,
--
-David
david(at)pgmasters(dot)net

Hi,

Am Dienstag, den 18.09.2018, 13:52 -0400 schrieb David Steele:
> On 9/18/18 11:45 AM, Stephen Frost wrote:
> > * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> > > I have added a retry for this as well now, without a pg_sleep() as well.
> > > This catches around 80% of the half-reads, but a few slip through. At
> > > that point we bail out with exit(1), and the user can try again, which I
> > > think is fine? 
> >
> > No, this is perfectly normal behavior, as is having completely blank
> > pages, now that I think about it. If we get a short read then I'd say
> > we simply check that we got an EOF and, in that case, we just move on.
> >
> > > Alternatively, we could just skip to the next file then and don't make
> > > it count as a checksum failure.
> >
> > No, I wouldn't count it as a checksum failure. We could possibly count
> > it towards the skipped pages, though I'm even on the fence about that.
>
> +1 for it not being a failure. Personally I'd count it as a skipped
> page, since we know the page exists but it can't be verified.
>
> The other option is to wait for the page to stabilize, which doesn't
> seem like it would take very long in most cases -- unless you are doing
> this test from another host with shared storage. Then I would expect to
> see all kinds of interesting torn pages after the last checkpoint.

OK, I'm skipping the block now on first try, as this makes (i) sense and
(ii) simplifies the code (again).

Version 3 is attached.

Michael

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

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

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

Hallo Michael,

Patch v3 applies cleanly, code compiles and make check is ok, but the
command is probably not tested anywhere, as already mentioned on other
threads.

The patch is missing a documentation update.

There are debatable changes of behavior:

if (errno == ENOENT) return / continue...

For instance, a file disappearing is ok online, but not so if offline. On
the other hand, the probability that a file suddenly disappears while the
server offline looks remote, so reporting such issues does not seem
useful.

However I'm more wary with other continues/skips added. ISTM that skipping
a block because of a read error, or because it is new, or some other
reasons, is not the same thing, so should be counted & reported
differently?

+ if (block_retry == false)

Why not trust boolean operations?

if (!block_retry)

--
Fabien.

Hi,

Am Mittwoch, den 26.09.2018, 13:23 +0200 schrieb Fabien COELHO:
> Patch v3 applies cleanly, code compiles and make check is ok, but the
> command is probably not tested anywhere, as already mentioned on other
> threads.

Right.

> The patch is missing a documentation update.

I've added that now. I think the only change needed was removing the
"server needs to be offline" part?

> There are debatable changes of behavior:
>
> if (errno == ENOENT) return / continue...
>
> For instance, a file disappearing is ok online, but not so if offline. On
> the other hand, the probability that a file suddenly disappears while the
> server offline looks remote, so reporting such issues does not seem
> useful.
>
> However I'm more wary with other continues/skips added. ISTM that skipping
> a block because of a read error, or because it is new, or some other
> reasons, is not the same thing, so should be counted & reported
> differently?

I think that would complicate things further without a lot of benefit.

After all, we are interested in checksum failures, not necessarily read
failures etc. so exiting on them (and skip checking possibly large parts
of PGDATA) looks undesirable to me.

So I have done no changes in this part so far, what do others think
about this?

> + if (block_retry == false)
>
> Why not trust boolean operations?
>
> if (!block_retry)

I've changed that as well.

Version 4 is attached.

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> Am Mittwoch, den 26.09.2018, 13:23 +0200 schrieb Fabien COELHO:
> > There are debatable changes of behavior:
> >
> > if (errno == ENOENT) return / continue...
> >
> > For instance, a file disappearing is ok online, but not so if offline. On
> > the other hand, the probability that a file suddenly disappears while the
> > server offline looks remote, so reporting such issues does not seem
> > useful.
> >
> > However I'm more wary with other continues/skips added. ISTM that skipping
> > a block because of a read error, or because it is new, or some other
> > reasons, is not the same thing, so should be counted & reported
> > differently?
>
> I think that would complicate things further without a lot of benefit.
>
> After all, we are interested in checksum failures, not necessarily read
> failures etc. so exiting on them (and skip checking possibly large parts
> of PGDATA) looks undesirable to me.
>
> So I have done no changes in this part so far, what do others think
> about this?

I certainly don't see a lot of point in doing much more than what was
discussed previously for 'new' blocks (counting them as skipped and
moving on).

An actual read() error (that is, a failure on a read() call such as
getting back EIO), on the other hand, is something which I'd probably
report back to the user immediately and then move on, and perhaps
report again at the end.

Note that a short read isn't an error and falls under the 'new' blocks
discussion above.

Thanks!

Stephen

>> The patch is missing a documentation update.
>
> I've added that now. I think the only change needed was removing the
> "server needs to be offline" part?

Yes, and also checking that the described behavior correspond to the new
version.

>> There are debatable changes of behavior:
>>
>> if (errno == ENOENT) return / continue...
>>
>> For instance, a file disappearing is ok online, but not so if offline. On
>> the other hand, the probability that a file suddenly disappears while the
>> server offline looks remote, so reporting such issues does not seem
>> useful.
>>
>> However I'm more wary with other continues/skips added. ISTM that skipping
>> a block because of a read error, or because it is new, or some other
>> reasons, is not the same thing, so should be counted & reported
>> differently?
>
> I think that would complicate things further without a lot of benefit.
>
> After all, we are interested in checksum failures, not necessarily read
> failures etc. so exiting on them (and skip checking possibly large parts
> of PGDATA) looks undesirable to me.

Hmmm.

I'm really saying that it is debatable, so here is some fuel to the
debate:

If I run the check command and it cannot do its job, there is a problem
which is as bad as a failing checksum. The only safe assumption on a
cannot-read block is that the checksum is bad... So ISTM that on
on some of the "skipped" errors there should be appropriate report (exit
code, final output) that something is amiss.

--
Fabien.

Hi,

Am Mittwoch, den 26.09.2018, 10:54 -0400 schrieb Stephen Frost:
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> > Am Mittwoch, den 26.09.2018, 13:23 +0200 schrieb Fabien COELHO:
> > > There are debatable changes of behavior:
> > >
> > > if (errno == ENOENT) return / continue...
> > >
> > > For instance, a file disappearing is ok online, but not so if offline. On
> > > the other hand, the probability that a file suddenly disappears while the
> > > server offline looks remote, so reporting such issues does not seem
> > > useful.
> > >
> > > However I'm more wary with other continues/skips added. ISTM that skipping
> > > a block because of a read error, or because it is new, or some other
> > > reasons, is not the same thing, so should be counted & reported
> > > differently?
> >
> > I think that would complicate things further without a lot of benefit.
> >
> > After all, we are interested in checksum failures, not necessarily read
> > failures etc. so exiting on them (and skip checking possibly large parts
> > of PGDATA) looks undesirable to me.
> >
> > So I have done no changes in this part so far, what do others think
> > about this?
>
> I certainly don't see a lot of point in doing much more than what was
> discussed previously for 'new' blocks (counting them as skipped and
> moving on).
>
> An actual read() error (that is, a failure on a read() call such as
> getting back EIO), on the other hand, is something which I'd probably
> report back to the user immediately and then move on, and perhaps
> report again at the end.
>
> Note that a short read isn't an error and falls under the 'new' blocks
> discussion above.

So I've added ENOENT checks when opening or statting files, i.e. EIO
would still be reported.

The current code in master exits on reads which do not return BLCKSZ,
which I've changed to a skip. So that means we now no longer check for
read failures (return code < 0) so I have now added a check for that and
emit an error message and return.

New version 5 attached.

Michael

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

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

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

Hello Stephen,

> I certainly don't see a lot of point in doing much more than what was
> discussed previously for 'new' blocks (counting them as skipped and
> moving on).

Sure.

> An actual read() error (that is, a failure on a read() call such as
> getting back EIO), on the other hand, is something which I'd probably
> report back to the user immediately and then move on, and perhaps
> report again at the end.

Yep.

> Note that a short read isn't an error and falls under the 'new' blocks
> discussion above.

I'm really unsure that a short read should really be coldly skipped:

If the check is offline, then one file is in a very bad state, this is
really a panic situation.

If the check is online, given that both postgres and the verify command
interact with the same OS (?) and at the pg page level, I'm not sure in
which situation there could be a partial block, because pg would only
send full pages to the OS.

--
Fabien.

Greetings,

* Fabien COELHO (coelho(at)cri(dot)ensmp(dot)fr) wrote:
> >Note that a short read isn't an error and falls under the 'new' blocks
> >discussion above.
>
> I'm really unsure that a short read should really be coldly skipped:
>
> If the check is offline, then one file is in a very bad state, this is
> really a panic situation.

Why? Are we sure that's really something which can't ever happen, even
if the database was shutdown with 'immediate'? I don't think it can but
that's something to consider. In any case, my comments were
specifically thinking about it from an 'online' perspective.

> If the check is online, given that both postgres and the verify command
> interact with the same OS (?) and at the pg page level, I'm not sure in
> which situation there could be a partial block, because pg would only send
> full pages to the OS.

The OS doesn't operate at the same level that PG does- a single write in
PG could get blocked and scheduled off after having only copied half of
the 8k that PG sends. This isn't really debatable- we've seen it happen
and everything is operating perfectly correctly, it just happens that
you were able to get a read() at the same time a write() was happening
and that only part of the page had been updated at that point.

Thanks!

Stephen

Hi,

On 09/26/2018 05:15 PM, Michael Banck wrote:
> ...
>
> New version 5 attached.
>

I've looked at v5, and the retry/recheck logic seems OK to me - I'd
still vote to keep it consistent with what pg_basebackup does (i.e.
doing the LSN check first, before looking at the checksum), but I don't
think it's a bug.

I'm not sure about the other issues brought up (ENOENT, short reads). I
haven't given it much thought.

regards

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

Hi,

One more thought - when running similar tools on a live system, it's
usually a good idea to limit the impact by throttling the throughput. As
the verification runs in an independent process it can't reuse the
vacuum-like cost limit directly, but perhaps it could do something
similar? Like, limit the number of blocks read/second, or so?

regards

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

On Sat, Sep 29, 2018 at 10:51:23AM +0200, Tomas Vondra wrote:
> One more thought - when running similar tools on a live system, it's
> usually a good idea to limit the impact by throttling the throughput. As
> the verification runs in an independent process it can't reuse the
> vacuum-like cost limit directly, but perhaps it could do something
> similar? Like, limit the number of blocks read/second, or so?

When it comes to such parameters, not using a number of blocks but
throttling with a value in bytes (kB or MB of course) speaks more to the
user. The past experience with checkpoint_segments is one example of
that. Converting that to a number of blocks internally would definitely
make sense the most sense. +1 for this idea.
--
Michael

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> On Sat, Sep 29, 2018 at 10:51:23AM +0200, Tomas Vondra wrote:
> > One more thought - when running similar tools on a live system, it's
> > usually a good idea to limit the impact by throttling the throughput. As
> > the verification runs in an independent process it can't reuse the
> > vacuum-like cost limit directly, but perhaps it could do something
> > similar? Like, limit the number of blocks read/second, or so?
>
> When it comes to such parameters, not using a number of blocks but
> throttling with a value in bytes (kB or MB of course) speaks more to the
> user. The past experience with checkpoint_segments is one example of
> that. Converting that to a number of blocks internally would definitely
> make sense the most sense. +1 for this idea.

While I agree this would be a nice additional feature to have, it seems
like something which could certainly be added later and doesn't
necessairly have to be included in the initial patch. If Michael has
time to add that, great, if not, I'd rather have this as-is than not.

I do tend to agree with Michael that having the parameter be specified
as (or at least able to accept) a byte-based value is a good idea. As
another feature idea, having this able to work in parallel across
tablespaces would be nice too. I can certainly imagine some point where
this is a default process which scans the database at a slow pace across
all the tablespaces more-or-less all the time checking for corruption.

Thanks!

Stephen

On 09/29/2018 02:14 PM, Stephen Frost wrote:
> Greetings,
>
> * Michael Paquier (michael(at)paquier(dot)xyz) wrote:
>> On Sat, Sep 29, 2018 at 10:51:23AM +0200, Tomas Vondra wrote:
>>> One more thought - when running similar tools on a live system, it's
>>> usually a good idea to limit the impact by throttling the throughput. As
>>> the verification runs in an independent process it can't reuse the
>>> vacuum-like cost limit directly, but perhaps it could do something
>>> similar? Like, limit the number of blocks read/second, or so?
>>
>> When it comes to such parameters, not using a number of blocks but
>> throttling with a value in bytes (kB or MB of course) speaks more to the
>> user. The past experience with checkpoint_segments is one example of
>> that. Converting that to a number of blocks internally would definitely
>> make sense the most sense. +1 for this idea.
>
> While I agree this would be a nice additional feature to have, it seems
> like something which could certainly be added later and doesn't
> necessairly have to be included in the initial patch. If Michael has
> time to add that, great, if not, I'd rather have this as-is than not.
>

True, although I don't think it'd be particularly difficult.

> I do tend to agree with Michael that having the parameter be specified
> as (or at least able to accept) a byte-based value is a good idea.

Sure, I was not really expecting it to be exposed as raw block count. I
agree it should be in byte-based values (i.e. just like --max-rate in
pg_basebackup).

> As another feature idea, having this able to work in parallel across
> tablespaces would be nice too. I can certainly imagine some point where
> this is a default process which scans the database at a slow pace across
> all the tablespaces more-or-less all the time checking for corruption.
>

Maybe, but that's certainly a non-trivial feature.

regards

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

Hallo Michael,

> New version 5 attached.

Patch does not seem to apply anymore.

Moreover, ISTM that some discussions about behavioral changes are not
fully settled.

My current opinion is that when offline some errors are not admissible,
whereas the same errors are admissible when online because they may be due
to the ongoing database processing, so the behavior should not be strictly
the same.

This might suggest some option to tell the command that it should work in
online or offline mode, so that it may be stricter in some cases. The
default may be one of the option, eg the stricter offline mode, or maybe
guessed at startup.

I put the patch in "waiting on author" state.

--
Fabien.

Hi Fabien,

On Thu, Oct 25, 2018 at 10:16:03AM +0200, Fabien COELHO wrote:
> >New version 5 attached.
>
> Patch does not seem to apply anymore.

Thanks, rebased version attached.

> Moreover, ISTM that some discussions about behavioral changes are not fully
> settled.
>
> My current opinion is that when offline some errors are not admissible,
> whereas the same errors are admissible when online because they may be due
> to the ongoing database processing, so the behavior should not be strictly
> the same.

Indeed, the recently-added pg_verify_checksums testsuite adds a few
files with just 'foo' in them and with V5 of the patch,
pg_verify_checksums no longer bails out with an error on those.

I have now re-added the retry logic for partially-read pages, so that it
bails out if it reads a page partially twice. This makes the testsuite
work again.

I am not convinced we need to differentiate further between online and
offline operation, can you explain in more detail which other
differences are ok in online mode and why?

> This might suggest some option to tell the command that it should work in
> online or offline mode, so that it may be stricter in some cases. The
> default may be one of the option, eg the stricter offline mode, or maybe
> guessed at startup.

If we believe the operation should be different, the patch removes the
"is cluster online?" check (as it is no longer necessary), so we could
just replace the current error message with a global variable with the
result of that check and use it where needed (if any).

Michael

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

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

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

Hallo Michael,

Patch v6 applies cleanly, compiles, local make check is ok.

>> My current opinion is that when offline some errors are not admissible,
>> whereas the same errors are admissible when online because they may be due
>> to the ongoing database processing, so the behavior should not be strictly
>> the same.
>
> Indeed, the recently-added pg_verify_checksums testsuite

A welcome addition!

> adds a few files with just 'foo' in them and with V5 of the patch,
> pg_verify_checksums no longer bails out with an error on those.

> I have now re-added the retry logic for partially-read pages, so that it
> bails out if it reads a page partially twice. This makes the testsuite
> work again.
>
> I am not convinced we need to differentiate further between online and
> offline operation, can you explain in more detail which other
> differences are ok in online mode and why?

For instance the "file/directory was removed" do not look okay at all when
offline, even if unlikely. Moreover, the checks hides the error message
and is fully silent in this case, while it was not beforehand on the
same error when offline.

The "check if page was modified since checkpoint" does not look useful
when offline. Maybe it lacks a comment to say that this cannot (should not
?) happen when offline, but even then I would not like it to be true: ISTM
that no page should be allowed to be skipped on the checkpoint condition
when offline, but it is probably ok to skip with the new page test, which
make me still think that they should be counted and reported separately,
or at least the checkpoint skip test should not be run when offline.

When offline, the retry logic does not make much sense, it should complain
directly on the first error? Also, I'm unsure of the read & checksum retry
logic *without any delay*.

>> This might suggest some option to tell the command that it should work in
>> online or offline mode, so that it may be stricter in some cases. The
>> default may be one of the option, eg the stricter offline mode, or maybe
>> guessed at startup.
>
> If we believe the operation should be different, the patch removes the
> "is cluster online?" check (as it is no longer necessary), so we could
> just replace the current error message with a global variable with the
> result of that check and use it where needed (if any).

That could let open the issue of someone starting the check offline, and
then starting the database while it is not finished. Maybe it is not worth
sweating about such a narrow use case.

If operations are to be different, and it seems to me they should be, I'd
suggest (1) auto detect default based one the existing "is cluster online"
code, (2) force options, eg --online vs --offline, which would complain
and exit if the cluster is not in the right state on startup.

I'd suggest to add a failing checksum online test, if possible. At least a
"foo" file? It would also be nice if the test could apply on an active
database, eg with a low-rate pgbench running in parallel to the
verification, but I'm not sure how easy it is to add such a thing.

--
Fabien.

Hi,

On Tue, Oct 30, 2018 at 06:22:52PM +0100, Fabien COELHO wrote:
> >I am not convinced we need to differentiate further between online and
> >offline operation, can you explain in more detail which other
> >differences are ok in online mode and why?
>
> For instance the "file/directory was removed" do not look okay at all when
> offline, even if unlikely. Moreover, the checks hides the error message and
> is fully silent in this case, while it was not beforehand on the same error
> when offline.

OK, I kinda see the point here and added that.

> The "check if page was modified since checkpoint" does not look useful when
> offline. Maybe it lacks a comment to say that this cannot (should not ?)
> happen when offline, but even then I would not like it to be true: ISTM that
> no page should be allowed to be skipped on the checkpoint condition when
> offline, but it is probably ok to skip with the new page test, which make me
> still think that they should be counted and reported separately, or at least
> the checkpoint skip test should not be run when offline.

What is the rationale to not skip on the checkpoint condition when the
instance is offline? If it was shutdown cleanly, this should not
happen, if the instance crashed, those would be spurious errors that
would get repaired on recovery.

I have not changed that for now.

> When offline, the retry logic does not make much sense, it should complain
> directly on the first error? Also, I'm unsure of the read & checksum retry
> logic *without any delay*.

I think the small overhead of retrying in offline mode even if useless
is worth avoiding making the code more complicated in order to cater for
both modes.

Initially there was a delay, but this was removed after analysis and
requests by several other reviewers.

> >>This might suggest some option to tell the command that it should work in
> >>online or offline mode, so that it may be stricter in some cases. The
> >>default may be one of the option, eg the stricter offline mode, or maybe
> >>guessed at startup.
> >
> >If we believe the operation should be different, the patch removes the
> >"is cluster online?" check (as it is no longer necessary), so we could
> >just replace the current error message with a global variable with the
> >result of that check and use it where needed (if any).
>
> That could let open the issue of someone starting the check offline, and
> then starting the database while it is not finished. Maybe it is not worth
> sweating about such a narrow use case.

I don't think we need to cater for that, yeah.

> If operations are to be different, and it seems to me they should be, I'd
> suggest (1) auto detect default based one the existing "is cluster online"
> code, (2) force options, eg --online vs --offline, which would complain and
> exit if the cluster is not in the right state on startup.

The current code bails out if it thinks the cluster is online. What is
wrong with just setting a flag now in case it is?

> I'd suggest to add a failing checksum online test, if possible. At least a
> "foo" file?

Ok, done so.

> It would also be nice if the test could apply on an active database,
> eg with a low-rate pgbench running in parallel to the verification,
> but I'm not sure how easy it is to add such a thing.

That sounds much more complicated so I have not tackled that yet.

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> On Tue, Oct 30, 2018 at 06:22:52PM +0100, Fabien COELHO wrote:
> > The "check if page was modified since checkpoint" does not look useful when
> > offline. Maybe it lacks a comment to say that this cannot (should not ?)
> > happen when offline, but even then I would not like it to be true: ISTM that
> > no page should be allowed to be skipped on the checkpoint condition when
> > offline, but it is probably ok to skip with the new page test, which make me
> > still think that they should be counted and reported separately, or at least
> > the checkpoint skip test should not be run when offline.
>
> What is the rationale to not skip on the checkpoint condition when the
> instance is offline? If it was shutdown cleanly, this should not
> happen, if the instance crashed, those would be spurious errors that
> would get repaired on recovery.
>
> I have not changed that for now.

Agreed- this is an important check even in offline mode.

> > When offline, the retry logic does not make much sense, it should complain
> > directly on the first error? Also, I'm unsure of the read & checksum retry
> > logic *without any delay*.

The race condition being considered here is where an 8k read somehow
gets the first 4k, then is scheduled off-cpu, and the full 8k page is
then written by some other process, and then this process is woken up
to read the second 4k. I agree that this is unnecessary when the
database is offline, but it's also pretty cheap. When the database is
online, it's an extremely unlikely case to hit (just try to reproduce
it...) but if it does get hit then it's easy enough to recheck by doing
a reread, which should show that the LSN has been updated in the first
4k and we can then know that this page is in the WAL. We have not yet
seen a case where such a re-read returns an old LSN and an invalid
checksum; based on discussion with other hackers, that shouldn't be
possible as every kernel seems to consistently write in-order, meaning
that the first 4k will be updated before the second, so a single re-read
should be sufficient.

Remember- this is all in-memory activity also, we aren't talking about
what might happen on disk here.

> I think the small overhead of retrying in offline mode even if useless
> is worth avoiding making the code more complicated in order to cater for
> both modes.

Agreed.

> Initially there was a delay, but this was removed after analysis and
> requests by several other reviewers.

Agreed, there's no need for or point to having such a delay.

> > >>This might suggest some option to tell the command that it should work in
> > >>online or offline mode, so that it may be stricter in some cases. The
> > >>default may be one of the option, eg the stricter offline mode, or maybe
> > >>guessed at startup.
> > >
> > >If we believe the operation should be different, the patch removes the
> > >"is cluster online?" check (as it is no longer necessary), so we could
> > >just replace the current error message with a global variable with the
> > >result of that check and use it where needed (if any).
> >
> > That could let open the issue of someone starting the check offline, and
> > then starting the database while it is not finished. Maybe it is not worth
> > sweating about such a narrow use case.
>
> I don't think we need to cater for that, yeah.

Agreed.

> > It would also be nice if the test could apply on an active database,
> > eg with a low-rate pgbench running in parallel to the verification,
> > but I'm not sure how easy it is to add such a thing.
>
> That sounds much more complicated so I have not tackled that yet.

I agree that this would be nice, but I don't want the regression tests
to become much longer...

Thanks!

Stephen

On 11/22/18 2:12 AM, Stephen Frost wrote:
> Greetings,
>
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
>> On Tue, Oct 30, 2018 at 06:22:52PM +0100, Fabien COELHO wrote:
>>> The "check if page was modified since checkpoint" does not look useful when
>>> offline. Maybe it lacks a comment to say that this cannot (should not ?)
>>> happen when offline, but even then I would not like it to be true: ISTM that
>>> no page should be allowed to be skipped on the checkpoint condition when
>>> offline, but it is probably ok to skip with the new page test, which make me
>>> still think that they should be counted and reported separately, or at least
>>> the checkpoint skip test should not be run when offline.
>>
>> What is the rationale to not skip on the checkpoint condition when the
>> instance is offline? If it was shutdown cleanly, this should not
>> happen, if the instance crashed, those would be spurious errors that
>> would get repaired on recovery.
>>
>> I have not changed that for now.
>
> Agreed- this is an important check even in offline mode.
>

Yeah. I suppose we could detect if the shutdown was clean (like
pg_rewind does), and then skip the check. Or perhaps we should still do
the check (without a retry), and report it as issue when we find a page
with LSN newer than the last checkpoint.

In any case, the check is pretty cheap (comparing two 64-bit values),
and I don't see how skipping it would optimize anything. It would make
the code a tad simpler, but we still need the check for the online mode.

>>> When offline, the retry logic does not make much sense, it should complain
>>> directly on the first error? Also, I'm unsure of the read & checksum retry
>>> logic *without any delay*.
>
> The race condition being considered here is where an 8k read somehow
> gets the first 4k, then is scheduled off-cpu, and the full 8k page is
> then written by some other process, and then this process is woken up
> to read the second 4k. I agree that this is unnecessary when the
> database is offline, but it's also pretty cheap. When the database is
> online, it's an extremely unlikely case to hit (just try to reproduce
> it...) but if it does get hit then it's easy enough to recheck by doing
> a reread, which should show that the LSN has been updated in the first
> 4k and we can then know that this page is in the WAL. We have not yet
> seen a case where such a re-read returns an old LSN and an invalid
> checksum; based on discussion with other hackers, that shouldn't be
> possible as every kernel seems to consistently write in-order, meaning
> that the first 4k will be updated before the second, so a single re-read
> should be sufficient.
>

Right.

A minor detail is that the reads/writes should be atomic at the sector
level, which used to be 512B, so it's not just about pages torn in
4kB/4kB manner, but possibly an arbitrary mix of 512B chunks from old
and new version.

This also explains why we don't need any delay - the reread happens
after the write must have already written the page header, so the new
LSN must be already visible.

So no delay is necessary. And if it was, how long should the delay be?
The processes might end up off-CPU for arbitrary amount of time, so
picking a good value would be pretty tricky.

> Remember- this is all in-memory activity also, we aren't talking about
> what might happen on disk here.
>
>> I think the small overhead of retrying in offline mode even if useless
>> is worth avoiding making the code more complicated in order to cater for
>> both modes.
>
> Agreed.
>
>> Initially there was a delay, but this was removed after analysis and
>> requests by several other reviewers.
>
> Agreed, there's no need for or point to having such a delay.
>

Yep.

>>>>> This might suggest some option to tell the command that it should work in
>>>>> online or offline mode, so that it may be stricter in some cases. The
>>>>> default may be one of the option, eg the stricter offline mode, or maybe
>>>>> guessed at startup.
>>>>
>>>> If we believe the operation should be different, the patch removes the
>>>> "is cluster online?" check (as it is no longer necessary), so we could
>>>> just replace the current error message with a global variable with the
>>>> result of that check and use it where needed (if any).
>>>
>>> That could let open the issue of someone starting the check offline, and
>>> then starting the database while it is not finished. Maybe it is not worth
>>> sweating about such a narrow use case.
>>
>> I don't think we need to cater for that, yeah.
>
> Agreed.
>

Yep. I don't think other tools protect against that either. And
pg_rewind does actually modify the cluster state, unlike checksum
verification.

>>> It would also be nice if the test could apply on an active database,
>>> eg with a low-rate pgbench running in parallel to the verification,
>>> but I'm not sure how easy it is to add such a thing.
>>
>> That sounds much more complicated so I have not tackled that yet.
>
> I agree that this would be nice, but I don't want the regression tests
> to become much longer...
>

I have to admit I find this thread rather confusing, because the subject
is "online verification of checksums" yet we're discussing verification
on offline instances.

regards

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

Greetings,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 11/22/18 2:12 AM, Stephen Frost wrote:
> >* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> >>On Tue, Oct 30, 2018 at 06:22:52PM +0100, Fabien COELHO wrote:
> >>>The "check if page was modified since checkpoint" does not look useful when
> >>>offline. Maybe it lacks a comment to say that this cannot (should not ?)
> >>>happen when offline, but even then I would not like it to be true: ISTM that
> >>>no page should be allowed to be skipped on the checkpoint condition when
> >>>offline, but it is probably ok to skip with the new page test, which make me
> >>>still think that they should be counted and reported separately, or at least
> >>>the checkpoint skip test should not be run when offline.
> >>
> >>What is the rationale to not skip on the checkpoint condition when the
> >>instance is offline? If it was shutdown cleanly, this should not
> >>happen, if the instance crashed, those would be spurious errors that
> >>would get repaired on recovery.
> >>
> >>I have not changed that for now.
> >
> >Agreed- this is an important check even in offline mode.
>
> Yeah. I suppose we could detect if the shutdown was clean (like pg_rewind
> does), and then skip the check. Or perhaps we should still do the check
> (without a retry), and report it as issue when we find a page with LSN newer
> than the last checkpoint.

I agree that it'd be nice to report an issue if it's a clean shutdown
but there's an LSN newer than the last checkpoint, though I suspect that
would be more useful in debugging and such and not so useful for users.

> In any case, the check is pretty cheap (comparing two 64-bit values), and I
> don't see how skipping it would optimize anything. It would make the code a
> tad simpler, but we still need the check for the online mode.

Yeah, I'd just keep the check.

> A minor detail is that the reads/writes should be atomic at the sector
> level, which used to be 512B, so it's not just about pages torn in 4kB/4kB
> manner, but possibly an arbitrary mix of 512B chunks from old and new
> version.

Sure.

> This also explains why we don't need any delay - the reread happens after
> the write must have already written the page header, so the new LSN must be
> already visible.

Agreed.

Thanks!

Stephen

> On Wed, Nov 21, 2018 at 1:38 PM Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
>
> Hi,
>
> On Tue, Oct 30, 2018 at 06:22:52PM +0100, Fabien COELHO wrote:
> > >I am not convinced we need to differentiate further between online and
> > >offline operation, can you explain in more detail which other
> > >differences are ok in online mode and why?
> >
> > For instance the "file/directory was removed" do not look okay at all when
> > offline, even if unlikely. Moreover, the checks hides the error message and
> > is fully silent in this case, while it was not beforehand on the same error
> > when offline.
>
> OK, I kinda see the point here and added that.

Hi,

Just for the information, looks like part of this patch (or at least some
similar code), related to the tests in 002_actions.pl, was committed recently
in 5c99513975, so there are minor conflicts with the master.

On Sat, Dec 01, 2018 at 12:47:13PM +0100, Dmitry Dolgov wrote:
> Just for the information, looks like part of this patch (or at least some
> similar code), related to the tests in 002_actions.pl, was committed recently
> in 5c99513975, so there are minor conflicts with the master.

What what I can see in v7 of the patch as posted in [1], all the changes
to 002_actions.pl could just be removed because there are already
equivalents.

[1]: https://postgr.es/m/20181121123535.GD23740@nighthawk.caipicrew.dd-dns.de
--
Michael

Hi,

On Mon, Dec 03, 2018 at 09:48:43AM +0900, Michael Paquier wrote:
> On Sat, Dec 01, 2018 at 12:47:13PM +0100, Dmitry Dolgov wrote:
> > Just for the information, looks like part of this patch (or at least some
> > similar code), related to the tests in 002_actions.pl, was committed recently
> > in 5c99513975, so there are minor conflicts with the master.
>
> What what I can see in v7 of the patch as posted in [1], all the changes
> to 002_actions.pl could just be removed because there are already
> equivalents.

Yeah, new rebased version attached.

Michael

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

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

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

Hi,

On Thu, Dec 20, 2018 at 04:19:11PM +0100, Michael Banck wrote:
> Yeah, new rebased version attached.

By the way, one thing that this patch also fixes is checksum
verification on basebackups (as pointed out the other day by my
colleague Bernd Helmele):

postgres(at)kohn:~$ initdb -k data
postgres(at)kohn:~$ pg_ctl -D data -l logfile start
waiting for server to start.... done
server started
postgres(at)kohn:~$ pg_verify_checksums -D data
pg_verify_checksums: cluster must be shut down to verify checksums
postgres(at)kohn:~$ pg_basebackup -h /tmp -D backup1
postgres(at)kohn:~$ pg_verify_checksums -D backup1
pg_verify_checksums: cluster must be shut down to verify checksums
postgres(at)kohn:~$ pg_checksums -c -D backup1
Checksum scan completed
Files scanned: 1094
Blocks scanned: 2867
Bad checksums: 0
Data checksum version: 1

Where pg_checksums has the online verification patch applied.

As I don't think many people will take down their production servers in
order to verify checksums, verifying them on basebackups looks like a
useful use-case that is currently broken with pg_verify_checksums.

Michael

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

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

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

Hallo Michael,

> Yeah, new rebased version attached.

Patch v8 applies cleanly, compiles, global & local make check are ok.

A few comments:

About added tests: the node is left running at the end of the script,
which is not very clean. I'd suggest to either move the added checks
before stopping, or to stop again at the end of the script, depending on
the intention.

I'm wondering (possibly again) about the existing early exit if one block
cannot be read on retry: the command should count this as a kind of bad
block, proceed on checking other files, and obviously fail in the end, but
having checked everything else and generated a report. I do not think that
this condition warrants a full stop. ISTM that under rare race conditions
(eg, an unlucky concurrent "drop database" or "drop table") this could
happen when online, although I could not trigger one despite heavy
testing, so I'm possibly mistaken.

--
Fabien.

Hi,

On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote:
> Hallo Michael,
>
> > Yeah, new rebased version attached.
>
> Patch v8 applies cleanly, compiles, global & local make check are ok.
>
> A few comments:
>
> About added tests: the node is left running at the end of the script, which
> is not very clean. I'd suggest to either move the added checks before
> stopping, or to stop again at the end of the script, depending on the
> intention.

Michael?

> I'm wondering (possibly again) about the existing early exit if one block
> cannot be read on retry: the command should count this as a kind of bad
> block, proceed on checking other files, and obviously fail in the end, but
> having checked everything else and generated a report. I do not think that
> this condition warrants a full stop. ISTM that under rare race conditions
> (eg, an unlucky concurrent "drop database" or "drop table") this could
> happen when online, although I could not trigger one despite heavy testing,
> so I'm possibly mistaken.

This seems like a defensible judgement call either way.

Greetings,

Andres Freund

On Sun, Feb 03, 2019 at 02:06:45AM -0800, Andres Freund wrote:
> On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote:
>> About added tests: the node is left running at the end of the script, which
>> is not very clean. I'd suggest to either move the added checks before
>> stopping, or to stop again at the end of the script, depending on the
>> intention.
>
> Michael?

Unlikely P., and most likely B.

I have marked the patch as returned with feedback as it has been a
couple of weeks already.
--
Michael

Hi,

Am Sonntag, den 03.02.2019, 02:06 -0800 schrieb Andres Freund:
> Hi,
>
> On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote:
> > Hallo Michael,
> >
> > > Yeah, new rebased version attached.
> >
> > Patch v8 applies cleanly, compiles, global & local make check are ok.
> >
> > A few comments:
> >
> > About added tests: the node is left running at the end of the script, which
> > is not very clean. I'd suggest to either move the added checks before
> > stopping, or to stop again at the end of the script, depending on the
> > intention.
>
> Michael?

Uh, I kinda forgot about this, I've made the tests stop the node now.

> > I'm wondering (possibly again) about the existing early exit if one block
> > cannot be read on retry: the command should count this as a kind of bad
> > block, proceed on checking other files, and obviously fail in the end, but
> > having checked everything else and generated a report. I do not think that
> > this condition warrants a full stop. ISTM that under rare race conditions
> > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > happen when online, although I could not trigger one despite heavy testing,
> > so I'm possibly mistaken.
>
> This seems like a defensible judgement call either way.

Right now we have a few tests that explicitly check that
pg_verify_checksums fail on broken data ("foo" in the file). Those
would then just get skipped AFAICT, which I think is the worse behaviour
, but if everybody thinks that should be the way to go, we can
drop/adjust those tests and make pg_verify_checksums skip them.

Thoughts?

In the meanwhile, v9 is attached with the above change and rebased
(without changes) to master.

Michael

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

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

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

Hallo Michael,

>>> I'm wondering (possibly again) about the existing early exit if one block
>>> cannot be read on retry: the command should count this as a kind of bad
>>> block, proceed on checking other files, and obviously fail in the end, but
>>> having checked everything else and generated a report. I do not think that
>>> this condition warrants a full stop. ISTM that under rare race conditions
>>> (eg, an unlucky concurrent "drop database" or "drop table") this could
>>> happen when online, although I could not trigger one despite heavy testing,
>>> so I'm possibly mistaken.
>>
>> This seems like a defensible judgement call either way.
>
> Right now we have a few tests that explicitly check that
> pg_verify_checksums fail on broken data ("foo" in the file). Those
> would then just get skipped AFAICT, which I think is the worse behaviour
> , but if everybody thinks that should be the way to go, we can
> drop/adjust those tests and make pg_verify_checksums skip them.
>
> Thoughts?

My point is that it should fail as it does, only not immediately (early
exit), but after having checked everything else. This mean avoiding
calling "exit(1)" here and there (lseek, fopen...), but taking note that
something bad happened, and call exit only in the end.

--
Fabien.

Hi,

On 2019-02-05 06:57:06 +0100, Fabien COELHO wrote:
> > > > I'm wondering (possibly again) about the existing early exit if one block
> > > > cannot be read on retry: the command should count this as a kind of bad
> > > > block, proceed on checking other files, and obviously fail in the end, but
> > > > having checked everything else and generated a report. I do not think that
> > > > this condition warrants a full stop. ISTM that under rare race conditions
> > > > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > > > happen when online, although I could not trigger one despite heavy testing,
> > > > so I'm possibly mistaken.
> > >
> > > This seems like a defensible judgement call either way.
> >
> > Right now we have a few tests that explicitly check that
> > pg_verify_checksums fail on broken data ("foo" in the file). Those
> > would then just get skipped AFAICT, which I think is the worse behaviour
> > , but if everybody thinks that should be the way to go, we can
> > drop/adjust those tests and make pg_verify_checksums skip them.
> >
> > Thoughts?
>
> My point is that it should fail as it does, only not immediately (early
> exit), but after having checked everything else. This mean avoiding calling
> "exit(1)" here and there (lseek, fopen...), but taking note that something
> bad happened, and call exit only in the end.

I can see both as being valuable (one gives you a more complete picture,
the other a quicker answer in scripts). For me that's the point where
it's the prerogative of the author to make that choice.

Greetings,

Andres Freund

On 2/5/19 8:01 AM, Andres Freund wrote:
> Hi,
>
> On 2019-02-05 06:57:06 +0100, Fabien COELHO wrote:
>>>>> I'm wondering (possibly again) about the existing early exit if one block
>>>>> cannot be read on retry: the command should count this as a kind of bad
>>>>> block, proceed on checking other files, and obviously fail in the end, but
>>>>> having checked everything else and generated a report. I do not think that
>>>>> this condition warrants a full stop. ISTM that under rare race conditions
>>>>> (eg, an unlucky concurrent "drop database" or "drop table") this could
>>>>> happen when online, although I could not trigger one despite heavy testing,
>>>>> so I'm possibly mistaken.
>>>>
>>>> This seems like a defensible judgement call either way.
>>>
>>> Right now we have a few tests that explicitly check that
>>> pg_verify_checksums fail on broken data ("foo" in the file). Those
>>> would then just get skipped AFAICT, which I think is the worse behaviour
>>> , but if everybody thinks that should be the way to go, we can
>>> drop/adjust those tests and make pg_verify_checksums skip them.
>>>
>>> Thoughts?
>>
>> My point is that it should fail as it does, only not immediately (early
>> exit), but after having checked everything else. This mean avoiding calling
>> "exit(1)" here and there (lseek, fopen...), but taking note that something
>> bad happened, and call exit only in the end.
>
> I can see both as being valuable (one gives you a more complete picture,
> the other a quicker answer in scripts). For me that's the point where
> it's the prerogative of the author to make that choice.
>

Why not make this configurable, using a command-line option?

regards

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

Hi,

Am Dienstag, den 05.02.2019, 11:30 +0100 schrieb Tomas Vondra:
> On 2/5/19 8:01 AM, Andres Freund wrote:
> > On 2019-02-05 06:57:06 +0100, Fabien COELHO wrote:
> > > > > > I'm wondering (possibly again) about the existing early exit if one block
> > > > > > cannot be read on retry: the command should count this as a kind of bad
> > > > > > block, proceed on checking other files, and obviously fail in the end, but
> > > > > > having checked everything else and generated a report. I do not think that
> > > > > > this condition warrants a full stop. ISTM that under rare race conditions
> > > > > > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > > > > > happen when online, although I could not trigger one despite heavy testing,
> > > > > > so I'm possibly mistaken.
> > > > >
> > > > > This seems like a defensible judgement call either way.
> > > >
> > > > Right now we have a few tests that explicitly check that
> > > > pg_verify_checksums fail on broken data ("foo" in the file). Those
> > > > would then just get skipped AFAICT, which I think is the worse behaviour
> > > > , but if everybody thinks that should be the way to go, we can
> > > > drop/adjust those tests and make pg_verify_checksums skip them.
> > > >
> > > > Thoughts?
> > >
> > > My point is that it should fail as it does, only not immediately (early
> > > exit), but after having checked everything else. This mean avoiding calling
> > > "exit(1)" here and there (lseek, fopen...), but taking note that something
> > > bad happened, and call exit only in the end.
> >
> > I can see both as being valuable (one gives you a more complete picture,
> > the other a quicker answer in scripts). For me that's the point where
> > it's the prerogative of the author to make that choice.

Personally, I would prefer to keep it as simple as possible for now and
get this patch committed; in my opinion the behaviour is already like
this (early exit on corrupt files) so I don't think the online
verification patch should change this.

If we see complaints about this, then I'd be happy to change it
afterwards.

> Why not make this configurable, using a command-line option?

I like this even less - this tool is about verifying checksums, so
adding options on what to do when it encounters broken pages looks out-
of-scope to me. Unless we want to say it should generally abort on the
first issue (i.e. on wrong checksums as well).

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> Am Dienstag, den 05.02.2019, 11:30 +0100 schrieb Tomas Vondra:
> > On 2/5/19 8:01 AM, Andres Freund wrote:
> > > On 2019-02-05 06:57:06 +0100, Fabien COELHO wrote:
> > > > > > > I'm wondering (possibly again) about the existing early exit if one block
> > > > > > > cannot be read on retry: the command should count this as a kind of bad
> > > > > > > block, proceed on checking other files, and obviously fail in the end, but
> > > > > > > having checked everything else and generated a report. I do not think that
> > > > > > > this condition warrants a full stop. ISTM that under rare race conditions
> > > > > > > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > > > > > > happen when online, although I could not trigger one despite heavy testing,
> > > > > > > so I'm possibly mistaken.
> > > > > >
> > > > > > This seems like a defensible judgement call either way.
> > > > >
> > > > > Right now we have a few tests that explicitly check that
> > > > > pg_verify_checksums fail on broken data ("foo" in the file). Those
> > > > > would then just get skipped AFAICT, which I think is the worse behaviour
> > > > > , but if everybody thinks that should be the way to go, we can
> > > > > drop/adjust those tests and make pg_verify_checksums skip them.
> > > > >
> > > > > Thoughts?
> > > >
> > > > My point is that it should fail as it does, only not immediately (early
> > > > exit), but after having checked everything else. This mean avoiding calling
> > > > "exit(1)" here and there (lseek, fopen...), but taking note that something
> > > > bad happened, and call exit only in the end.
> > >
> > > I can see both as being valuable (one gives you a more complete picture,
> > > the other a quicker answer in scripts). For me that's the point where
> > > it's the prerogative of the author to make that choice.

... unless people here object or prefer other options, and then it's up
to discussion and hopefully some consensus comes out of it.

Also, I have to say that I really don't think the 'quicker answer'
argument holds any weight, making me question if that's a valid
use-case. If there *isn't* an issue, which we would likely all agree is
the case the vast majority of the time that this is going to be run,
then it's going to take quite a while and anyone calling it should
expect and be prepared for that. In the extremely rare cases, what does
exiting early actually do for us?

> Personally, I would prefer to keep it as simple as possible for now and
> get this patch committed; in my opinion the behaviour is already like
> this (early exit on corrupt files) so I don't think the online
> verification patch should change this.

I'm also in the camp of "would rather it not exit immediately, so the
extent of the issue is clear".

> If we see complaints about this, then I'd be happy to change it
> afterwards.

I really don't think this is something we should change later on in a
future release.. If the consensus is that there's really two different
but valid use-cases then we should make it configurable, but I'm not
convinced there is.

> > Why not make this configurable, using a command-line option?
>
> I like this even less - this tool is about verifying checksums, so
> adding options on what to do when it encounters broken pages looks out-
> of-scope to me. Unless we want to say it should generally abort on the
> first issue (i.e. on wrong checksums as well).

I definitely disagree that it's somehow 'out of scope' for this tool to
skip broken pages, when we can tell that they're broken. There is a
question here about how to handle a short read since that can happen
under normal conditions if we're unlucky. The same is also true for
files disappearing entirely.

So, let's talk/think through a few cases:

A file with just 'foo\n' in it- could that be a page starting with
an LSN around 666F6F0A that we somehow only read the first few bytes of?
If not, why not? I could possibly see an argument that we expect to
always get at least 512 bytes in a read, or 4K, but it seems like we
could possibly run into edge cases on odd filesystems or such. In the
end, I'm leaning towards categorizing different things, well,
differently- a short read would be reported as a NOTICE or equivilant,
perhaps, meaning that the test case needs to do something more than just
have a file with 'foo' in it, but that is likely a good things anyway-
the test cases would be better if they were closer to real world. Other
read failures would be reported in a more serious category assuming they
are "this really shouldn't happen" cases. A file disappearing isn't a
"can't happen" case, and might be reported at the same 'NOTICE' level
(or maybe with a 'verbose' ption).

A file that's 8k in size and has a checksum but it's not right seems
pretty clear to me. Might as well include a count of pages which have a
valid checksum, I would think, though perhaps only in a 'verbose' mode
would that get reported.

A completely zero'd page could also be reported at a NOTICE level or
with a count, or perhaps only with verbose.

Other thoughts about use-cases and what should happen..?

Thanks!

Stephen

Hi,

Am Mittwoch, den 06.02.2019, 11:39 -0500 schrieb Stephen Frost:
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> > Am Dienstag, den 05.02.2019, 11:30 +0100 schrieb Tomas Vondra:
> > > On 2/5/19 8:01 AM, Andres Freund wrote:
> > > > On 2019-02-05 06:57:06 +0100, Fabien COELHO wrote:
> > > > > > > > I'm wondering (possibly again) about the existing early exit if one block
> > > > > > > > cannot be read on retry: the command should count this as a kind of bad
> > > > > > > > block, proceed on checking other files, and obviously fail in the end, but
> > > > > > > > having checked everything else and generated a report. I do not think that
> > > > > > > > this condition warrants a full stop. ISTM that under rare race conditions
> > > > > > > > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > > > > > > > happen when online, although I could not trigger one despite heavy testing,
> > > > > > > > so I'm possibly mistaken.
> > > > > > >
> > > > > > > This seems like a defensible judgement call either way.
> > > > > >
> > > > > > Right now we have a few tests that explicitly check that
> > > > > > pg_verify_checksums fail on broken data ("foo" in the file). Those
> > > > > > would then just get skipped AFAICT, which I think is the worse behaviour
> > > > > > , but if everybody thinks that should be the way to go, we can
> > > > > > drop/adjust those tests and make pg_verify_checksums skip them.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > My point is that it should fail as it does, only not immediately (early
> > > > > exit), but after having checked everything else. This mean avoiding calling
> > > > > "exit(1)" here and there (lseek, fopen...), but taking note that something
> > > > > bad happened, and call exit only in the end.
> > > >
> > > > I can see both as being valuable (one gives you a more complete picture,
> > > > the other a quicker answer in scripts). For me that's the point where
> > > > it's the prerogative of the author to make that choice.
>
> ... unless people here object or prefer other options, and then it's up
> to discussion and hopefully some consensus comes out of it.
>
> Also, I have to say that I really don't think the 'quicker answer'
> argument holds any weight, making me question if that's a valid
> use-case. If there *isn't* an issue, which we would likely all agree is
> the case the vast majority of the time that this is going to be run,
> then it's going to take quite a while and anyone calling it should
> expect and be prepared for that. In the extremely rare cases, what does
> exiting early actually do for us?
>
> > Personally, I would prefer to keep it as simple as possible for now and
> > get this patch committed; in my opinion the behaviour is already like
> > this (early exit on corrupt files) so I don't think the online
> > verification patch should change this.
>
> I'm also in the camp of "would rather it not exit immediately, so the
> extent of the issue is clear".
>
> > If we see complaints about this, then I'd be happy to change it
> > afterwards.
>
> I really don't think this is something we should change later on in a
> future release.. If the consensus is that there's really two different
> but valid use-cases then we should make it configurable, but I'm not
> convinced there is.

OK, fair enough.

> > > Why not make this configurable, using a command-line option?
> >
> > I like this even less - this tool is about verifying checksums, so
> > adding options on what to do when it encounters broken pages looks out-
> > of-scope to me. Unless we want to say it should generally abort on the
> > first issue (i.e. on wrong checksums as well).
>
> I definitely disagree that it's somehow 'out of scope' for this tool to
> skip broken pages, when we can tell that they're broken.

I didn't mean that it's out-of-scope for pg_verify_checksums, I meant it
is out-of-scope for this patch, which adds online checking.

> There is a question here about how to handle a short read since that
> can happen under normal conditions if we're unlucky. The same is also
> true for files disappearing entirely.
>
> So, let's talk/think through a few cases:
>
> A file with just 'foo\n' in it- could that be a page starting with
> an LSN around 666F6F0A that we somehow only read the first few bytes of?
> If not, why not? I could possibly see an argument that we expect to
> always get at least 512 bytes in a read, or 4K, but it seems like we
> could possibly run into edge cases on odd filesystems or such. In the
> end, I'm leaning towards categorizing different things, well,
> differently- a short read would be reported as a NOTICE or equivilant,
> perhaps, meaning that the test case needs to do something more than just
> have a file with 'foo' in it, but that is likely a good things anyway-
> the test cases would be better if they were closer to real world. Other
> read failures would be reported in a more serious category assuming they
> are "this really shouldn't happen" cases. A file disappearing isn't a
> "can't happen" case, and might be reported at the same 'NOTICE' level
> (or maybe with a 'verbose' ption).

In the context of this patch, we should also discern whether a
particular case is merely a notice (or warning) on an offline cluster, I
guess you think it should be?

So I've changed it such that a short read emits a "warning" message,
increments a new skippedfiles (as it is not just a skipped block)
variable and reports its number at the end - should it then exit with >
0 even if there were no wrong checksums?

> A file that's 8k in size and has a checksum but it's not right seems
> pretty clear to me. Might as well include a count of pages which have a
> valid checksum, I would think, though perhaps only in a 'verbose' mode
> would that get reported.

What's the use for that? It already reports the number of scanned blocks
at the end, so that number is pretty easy to figure out from it and the
number of bad checksums.

> A completely zero'd page could also be reported at a NOTICE level or
> with a count, or perhaps only with verbose.

It is counted as a skipped block right now (well, every block that
qualifes for PageIsNew() is), but skipped blocks are not mentioned right
now. I guess the rationale is that it might lead to excessive screen
output (but then, verbose originally logged /every/ block), but you'd
have to check with the original authors.

So I have now changed behaviour so that short writes count as skipped
files and pg_verify_checksums no longer bails out on them. When this
occors a warning is written to stderr and their overall count is also
reported at the end. However, unless there are other blocks with bad
checksums, the exit status is kept at zero.

New patch attached.

Michael

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

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

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

Hallo Mickael,

> So I have now changed behaviour so that short writes count as skipped
> files and pg_verify_checksums no longer bails out on them. When this
> occors a warning is written to stderr and their overall count is also
> reported at the end. However, unless there are other blocks with bad
> checksums, the exit status is kept at zero.

This seems fair when online, however I'm wondering whether it is when
offline. I'd say that the whole retry logic should be skipped in this
case? i.e. "if (block_retry || !online) { error message and continue }"
on both short read & checksum failure retries.

> New patch attached.

Patch applies cleanly, compiles, global & local make check ok.

I'm wondering whether it should exit(1) on "lseek" failures. Would it make
sense to skip the file and report it as such? Should it be counted as a
skippedfile?

WRT the final status, ISTM that slippedblocks & files could warrant an
error when offline, although they might be ok when online?

--
Fabien.

Hi,

Am Donnerstag, den 28.02.2019, 14:29 +0100 schrieb Fabien COELHO:
> > So I have now changed behaviour so that short writes count as skipped
> > files and pg_verify_checksums no longer bails out on them. When this
> > occors a warning is written to stderr and their overall count is also
> > reported at the end. However, unless there are other blocks with bad
> > checksums, the exit status is kept at zero.
>
> This seems fair when online, however I'm wondering whether it is when
> offline. I'd say that the whole retry logic should be skipped in this
> case? i.e. "if (block_retry || !online) { error message and continue }"
> on both short read & checksum failure retries.

Ok, the stand-alone pg_checksums program also got a PR about the LSN
skip logic not being helpful when the instance is offline and somebody
just writes /dev/urandom over the heap files: 

https://github.com/credativ/pg_checksums/pull/6

So I now tried to change the patch so that it only retries blocks when
online.

> Patch applies cleanly, compiles, global & local make check ok.
>
> I'm wondering whether it should exit(1) on "lseek" failures. Would it make
> sense to skip the file and report it as such? Should it be counted as a
> skippedfile?

Ok, I think it makes sense to march on and I changed it that way.

> WRT the final status, ISTM that slippedblocks & files could warrant an
> error when offline, although they might be ok when online?

Ok, also changed it that way.

New patch attached.

Michael

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

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

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

On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
<michael(dot)banck(at)credativ(dot)de> wrote:
> I have added a retry for this as well now, without a pg_sleep() as well.
> This catches around 80% of the half-reads, but a few slip through. At
> that point we bail out with exit(1), and the user can try again, which I
> think is fine?

Maybe I'm confused here, but catching 80% of torn pages doesn't sound
robust at all.

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

Hi,

Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> <michael(dot)banck(at)credativ(dot)de> wrote:
> > I have added a retry for this as well now, without a pg_sleep() as well.
> > This catches around 80% of the half-reads, but a few slip through. At
> > that point we bail out with exit(1), and the user can try again, which I
> > think is fine?
>
> Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> robust at all.

The chance that pg_verify_checksums hits a torn page (at least in my
tests, see below) is already pretty low, a couple of times per 1000
runs. Maybe 4 out 5 times, the page is read fine on retry and we march
on. Otherwise, we now just issue a warning and skip the file (or so was
the idea, see below), do you think that is not acceptable?

I re-ran the tests (concurrent createdb/pgbench -i -s 50/dropdb and
pg_verify_checksums in tight loops) with the current patch version, and
I am seeing short reads very, very rarely (maybe every 1000th run) with
a warning like:

|1174
|pg_verify_checksums: warning: could not read block 374 in file "data/base/18032/18045": read 4096 of 8192
|pg_verify_checksums: warning: could not read block 375 in file "data/base/18032/18045": read 4096 of 8192
|Files skipped: 2

The 1174 is the sequence number, the first 1173 runs of
pg_verify_checksums only skipped blocks.

However, the fact it shows two warnings for the same file means there is
something wrong here. It was continueing to the next block while I think
it should just skip to the next file on read failures. So I have changed
that now, new patch attached.

Michael

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

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

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

Greetings,

* Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> > On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> > <michael(dot)banck(at)credativ(dot)de> wrote:
> > > I have added a retry for this as well now, without a pg_sleep() as well.
> > > This catches around 80% of the half-reads, but a few slip through. At
> > > that point we bail out with exit(1), and the user can try again, which I
> > > think is fine?
> >
> > Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> > robust at all.
>
> The chance that pg_verify_checksums hits a torn page (at least in my
> tests, see below) is already pretty low, a couple of times per 1000
> runs. Maybe 4 out 5 times, the page is read fine on retry and we march
> on. Otherwise, we now just issue a warning and skip the file (or so was
> the idea, see below), do you think that is not acceptable?
>
> I re-ran the tests (concurrent createdb/pgbench -i -s 50/dropdb and
> pg_verify_checksums in tight loops) with the current patch version, and
> I am seeing short reads very, very rarely (maybe every 1000th run) with
> a warning like:
>
> |1174
> |pg_verify_checksums: warning: could not read block 374 in file "data/base/18032/18045": read 4096 of 8192
> |pg_verify_checksums: warning: could not read block 375 in file "data/base/18032/18045": read 4096 of 8192
> |Files skipped: 2
>
> The 1174 is the sequence number, the first 1173 runs of
> pg_verify_checksums only skipped blocks.
>
> However, the fact it shows two warnings for the same file means there is
> something wrong here. It was continueing to the next block while I think
> it should just skip to the next file on read failures. So I have changed
> that now, new patch attached.

I'm confused- if previously it was continueing to the next block instead
of doing the re-read on the same block, why don't we just change it to
do the re-read on the same block properly and see if that fixes the
retry, instead of just giving up and skipping..? I'm not necessairly
against skipping to the next file, to be clear, but I think I'd be
happier if we kept reading the file until we actually get EOF.

(I've not looked at the actual patch, just read what you wrote..)

Thanks!

Stephen

On 3/2/19 12:03 AM, Robert Haas wrote:
> On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> <michael(dot)banck(at)credativ(dot)de> wrote:
>> I have added a retry for this as well now, without a pg_sleep() as well.
>> This catches around 80% of the half-reads, but a few slip through. At
>> that point we bail out with exit(1), and the user can try again, which I
>> think is fine?
>
> Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> robust at all.
>

FWIW I don't think this qualifies as torn page - i.e. it's not a full
read with a mix of old and new data. This is partial write, most likely
because we read the blocks one by one, and when we hit the last page
while the table is being extended, we may only see the fist 4kB. And if
we retry very fast, we may still see only the first 4kB.

regards

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

On 3/2/19 5:08 PM, Stephen Frost wrote:
> Greetings,
>
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
>> Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
>>> On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
>>> <michael(dot)banck(at)credativ(dot)de> wrote:
>>>> I have added a retry for this as well now, without a pg_sleep() as well.
>>>> This catches around 80% of the half-reads, but a few slip through. At
>>>> that point we bail out with exit(1), and the user can try again, which I
>>>> think is fine?
>>>
>>> Maybe I'm confused here, but catching 80% of torn pages doesn't sound
>>> robust at all.
>>
>> The chance that pg_verify_checksums hits a torn page (at least in my
>> tests, see below) is already pretty low, a couple of times per 1000
>> runs. Maybe 4 out 5 times, the page is read fine on retry and we march
>> on. Otherwise, we now just issue a warning and skip the file (or so was
>> the idea, see below), do you think that is not acceptable?
>>
>> I re-ran the tests (concurrent createdb/pgbench -i -s 50/dropdb and
>> pg_verify_checksums in tight loops) with the current patch version, and
>> I am seeing short reads very, very rarely (maybe every 1000th run) with
>> a warning like:
>>
>> |1174
>> |pg_verify_checksums: warning: could not read block 374 in file "data/base/18032/18045": read 4096 of 8192
>> |pg_verify_checksums: warning: could not read block 375 in file "data/base/18032/18045": read 4096 of 8192
>> |Files skipped: 2
>>
>> The 1174 is the sequence number, the first 1173 runs of
>> pg_verify_checksums only skipped blocks.
>>
>> However, the fact it shows two warnings for the same file means there is
>> something wrong here. It was continueing to the next block while I think
>> it should just skip to the next file on read failures. So I have changed
>> that now, new patch attached.
>
> I'm confused- if previously it was continueing to the next block instead
> of doing the re-read on the same block, why don't we just change it to
> do the re-read on the same block properly and see if that fixes the
> retry, instead of just giving up and skipping..? I'm not necessairly
> against skipping to the next file, to be clear, but I think I'd be
> happier if we kept reading the file until we actually get EOF.
>
> (I've not looked at the actual patch, just read what you wrote..)
>

Notice that those two errors are actually for two consecutive blocks in
the same file. So what probably happened is that postgres started to
extend the page, and the verification tried to read the last page after
the kernel added just the first 4kB filesystem page. Then it probably
succeeded on a retry, and then the same thing happened on the next page.

I don't think EOF addresses this, though - the partial read happens
before we actually reach the end of the file.

And re-reads are not a solution either, because the second read may
still see only the first half, and then what - is it a permanent issue
(in which case it's a data corruption), or an extension in progress?

I wonder if we can simply ignore those errors entirely, if it's the last
page in the segment? We can't really check the file is "complete"
anyway, e.g. if you have multiple segments for a table, and the "middle"
one is a page shorter, we'll happily ignore that during verification.

Also, what if we're reading a file and it gets truncated (e.g. after
vacuum notices the last few pages are empty)? Doesn't that have the same
issue?

regards

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

Hi,

On 2019-03-02 22:49:33 +0100, Tomas Vondra wrote:
>
>
> On 3/2/19 5:08 PM, Stephen Frost wrote:
> > Greetings,
> >
> > * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> >> Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> >>> On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> >>> <michael(dot)banck(at)credativ(dot)de> wrote:
> >>>> I have added a retry for this as well now, without a pg_sleep() as well.
> >>>> This catches around 80% of the half-reads, but a few slip through. At
> >>>> that point we bail out with exit(1), and the user can try again, which I
> >>>> think is fine?
> >>>
> >>> Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> >>> robust at all.
> >>
> >> The chance that pg_verify_checksums hits a torn page (at least in my
> >> tests, see below) is already pretty low, a couple of times per 1000
> >> runs. Maybe 4 out 5 times, the page is read fine on retry and we march
> >> on. Otherwise, we now just issue a warning and skip the file (or so was
> >> the idea, see below), do you think that is not acceptable?
> >>
> >> I re-ran the tests (concurrent createdb/pgbench -i -s 50/dropdb and
> >> pg_verify_checksums in tight loops) with the current patch version, and
> >> I am seeing short reads very, very rarely (maybe every 1000th run) with
> >> a warning like:
> >>
> >> |1174
> >> |pg_verify_checksums: warning: could not read block 374 in file "data/base/18032/18045": read 4096 of 8192
> >> |pg_verify_checksums: warning: could not read block 375 in file "data/base/18032/18045": read 4096 of 8192
> >> |Files skipped: 2
> >>
> >> The 1174 is the sequence number, the first 1173 runs of
> >> pg_verify_checksums only skipped blocks.
> >>
> >> However, the fact it shows two warnings for the same file means there is
> >> something wrong here. It was continueing to the next block while I think
> >> it should just skip to the next file on read failures. So I have changed
> >> that now, new patch attached.
> >
> > I'm confused- if previously it was continueing to the next block instead
> > of doing the re-read on the same block, why don't we just change it to
> > do the re-read on the same block properly and see if that fixes the
> > retry, instead of just giving up and skipping..? I'm not necessairly
> > against skipping to the next file, to be clear, but I think I'd be
> > happier if we kept reading the file until we actually get EOF.
> >
> > (I've not looked at the actual patch, just read what you wrote..)
> >
>
> Notice that those two errors are actually for two consecutive blocks in
> the same file. So what probably happened is that postgres started to
> extend the page, and the verification tried to read the last page after
> the kernel added just the first 4kB filesystem page. Then it probably
> succeeded on a retry, and then the same thing happened on the next page.
>
> I don't think EOF addresses this, though - the partial read happens
> before we actually reach the end of the file.
>
> And re-reads are not a solution either, because the second read may
> still see only the first half, and then what - is it a permanent issue
> (in which case it's a data corruption), or an extension in progress?
>
> I wonder if we can simply ignore those errors entirely, if it's the last
> page in the segment? We can't really check the file is "complete"
> anyway, e.g. if you have multiple segments for a table, and the "middle"
> one is a page shorter, we'll happily ignore that during verification.
>
> Also, what if we're reading a file and it gets truncated (e.g. after
> vacuum notices the last few pages are empty)? Doesn't that have the same
> issue?

I gotta say, my conclusion from this debate is that it's simply a
mistake to do this without involvement of the server that can use
locking to prevent these kind of issues. It seems pretty absurd to me
to have hacky workarounds around partial writes of a live server, around
truncation, etc, even though the server has ways to deal with that.

- Andres

On Sat, Mar 02, 2019 at 02:00:31PM -0800, Andres Freund wrote:
> I gotta say, my conclusion from this debate is that it's simply a
> mistake to do this without involvement of the server that can use
> locking to prevent these kind of issues. It seems pretty absurd to me
> to have hacky workarounds around partial writes of a live server, around
> truncation, etc, even though the server has ways to deal with that.

I agree with Andres on this one. We are never going to make this
stuff safe if we don't handle page reads with the proper locks because
of torn pages. What I think we should do is provide a SQL function
which reads a page in shared mode, and then checks its checksum if its
LSN is older than the previous redo point. This discards cases with
rather hot pages, but if the page is hot enough then the backend
re-reading the page would just do the same by verifying the page
checksum by itself.
--
Michael

On 3/3/19 12:48 AM, Michael Paquier wrote:
> On Sat, Mar 02, 2019 at 02:00:31PM -0800, Andres Freund wrote:
>> I gotta say, my conclusion from this debate is that it's simply a
>> mistake to do this without involvement of the server that can use
>> locking to prevent these kind of issues. It seems pretty absurd to me
>> to have hacky workarounds around partial writes of a live server, around
>> truncation, etc, even though the server has ways to deal with that.
>
> I agree with Andres on this one. We are never going to make this
> stuff safe if we don't handle page reads with the proper locks because
> of torn pages. What I think we should do is provide a SQL function
> which reads a page in shared mode, and then checks its checksum if its
> LSN is older than the previous redo point. This discards cases with
> rather hot pages, but if the page is hot enough then the backend
> re-reading the page would just do the same by verifying the page
> checksum by itself.

Handling torn pages is not difficult, and the patch already does that
(it reads LSN of the last checkpoint LSN from the control file, and uses
it the same way basebackup does). That's working since (at least)
September, so I don't see how would the SQL function help with this?

The other issue (raised recently) is partial reads, where we read only a
fraction of the page. Basebackup simply ignores such pages, likely on
the assumption that it's either concurrent extension or truncation (in
which case it's newer than the last checkpoint LSN anyway). So maybe we
should do the same thing here. As I mentioned before, we can't reliably
detect incomplete segments anyway (at least I believe that's the case).

You and Andres may be right that trying to verify checksums online
without close interaction with the server is ultimately futile (or at
least overly complex). But I'm not sure those issues (torn pages and
partial reads) are very good arguments, considering basebackup has to
deal with them too. Not sure.

regards

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

Bonjour Michaël,

>> I gotta say, my conclusion from this debate is that it's simply a
>> mistake to do this without involvement of the server that can use
>> locking to prevent these kind of issues. It seems pretty absurd to me
>> to have hacky workarounds around partial writes of a live server, around
>> truncation, etc, even though the server has ways to deal with that.
>
> I agree with Andres on this one. We are never going to make this stuff
> safe if we don't handle page reads with the proper locks because of torn
> pages. What I think we should do is provide a SQL function which reads a
> page in shared mode, and then checks its checksum if its LSN is older
> than the previous redo point. This discards cases with rather hot
> pages, but if the page is hot enough then the backend re-reading the
> page would just do the same by verifying the page checksum by itself. --
> Michael

My 0.02€ about that, as one of the reviewer of the patch:

I agree that having a server function (extension?) to do a full checksum
verification, possibly bandwidth-controlled, would be a good thing.
However it would have side effects, such as interfering deeply with the
server page cache, which may or may not be desirable.

On the other hand I also see value in an independent system-level external
tool capable of a best effort checksum verification: the current check
that the cluster is offline to prevent pg_verify_checksum from running is
kind of artificial, and when online simply counting
online-database-related checksum issues looks like a reasonable
compromise.

So basically I think that allowing pg_verify_checksum to run on an online
cluster is still a good thing, provided that expected errors are correctly
handled.

--
Fabien.

Hi,

Am Samstag, den 02.03.2019, 11:08 -0500 schrieb Stephen Frost:h
> * Michael Banck (michael(dot)banck(at)credativ(dot)de) wrote:
> > Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> > > On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> > > <michael(dot)banck(at)credativ(dot)de> wrote:
> > > > I have added a retry for this as well now, without a pg_sleep() as well.
> > > > This catches around 80% of the half-reads, but a few slip through. At
> > > > that point we bail out with exit(1), and the user can try again, which I
> > > > think is fine?
> > >
> > > Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> > > robust at all.
> >
> > The chance that pg_verify_checksums hits a torn page (at least in my
> > tests, see below) is already pretty low, a couple of times per 1000
> > runs. Maybe 4 out 5 times, the page is read fine on retry and we march
> > on. Otherwise, we now just issue a warning and skip the file (or so was
> > the idea, see below), do you think that is not acceptable?
> >
> > I re-ran the tests (concurrent createdb/pgbench -i -s 50/dropdb and
> > pg_verify_checksums in tight loops) with the current patch version, and
> > I am seeing short reads very, very rarely (maybe every 1000th run) with
> > a warning like:
> >
> > > 1174
> > > pg_verify_checksums: warning: could not read block 374 in file "data/base/18032/18045": read 4096 of 8192
> > > pg_verify_checksums: warning: could not read block 375 in file "data/base/18032/18045": read 4096 of 8192
> > > Files skipped: 2
> >
> > The 1174 is the sequence number, the first 1173 runs of
> > pg_verify_checksums only skipped blocks.
> >
> > However, the fact it shows two warnings for the same file means there is
> > something wrong here. It was continueing to the next block while I think
> > it should just skip to the next file on read failures. So I have changed
> > that now, new patch attached.
>
> I'm confused- if previously it was continueing to the next block instead
> of doing the re-read on the same block, why don't we just change it to
> do the re-read on the same block properly and see if that fixes the
> retry, instead of just giving up and skipping..?

It was re-reading the block and continueing to read the file after it
got a short read even on re-read.

> I'm not necessairly against skipping to the next file, to be clear,
> but I think I'd be happier if we kept reading the file until we
> actually get EOF.

So if we read half a block twice we should seek() to the next block and
continue till EOF, ok. I think in most cases those pages will be new
anyway and there will be no checksum check, but it sounds like a cleaner
approach. I've seen one or two examples where we did successfully verify
the checksum of a page after a half-read, so it might be worth it.

The alternative would be to just bail out early and skip the file on the
first short read and (possibly) log a skipped file.

I still think that an external checksum verification tool has some
merit, given that basebackup does it and the current offline requirement
is really not useful in practise.

Michael

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

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

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

On Sun, Mar 03, 2019 at 03:12:51AM +0100, Tomas Vondra wrote:
> You and Andres may be right that trying to verify checksums online
> without close interaction with the server is ultimately futile (or at
> least overly complex). But I'm not sure those issues (torn pages and
> partial reads) are very good arguments, considering basebackup has to
> deal with them too. Not sure.

FWIW, I don't think that the backend is right in its way of checking
checksums the way it does currently either with warnings and a limited
set of failures generated. I raised concerns about that unfortunately
after 11 has been GA'ed, which was too late, so this time, for this
patch, I prefer raising them before the fact and I'd rather not spread
this kind of methodology around the core code more and more. I work a
lot with virtualization, and I have seen ESX hanging around I/O
requests from time to time depending on the environment used (which is
actually wrong, anyway, but a lot of tests happen on a daily basis on
the stuff I work on). What's presented on this thread is *never*
going to be 100% safe, and would generate false positives which can be
confusing for the user. This is not a good sign.
--
Michael

On Sun, Mar 03, 2019 at 11:51:48AM +0100, Michael Banck wrote:
> I still think that an external checksum verification tool has some
> merit, given that basebackup does it and the current offline requirement
> is really not useful in practise.

I am not going to argue again about the way checksum verification is
done in a base backup.. :)

Being able to do an online verification of checksums has a lot of
value, do not take me wrong, and an SQL interface to do that does not
prevent having a frontend wrapper using it.
--
Michael

On Sun, Mar 03, 2019 at 07:58:26AM +0100, Fabien COELHO wrote:
> I agree that having a server function (extension?) to do a full checksum
> verification, possibly bandwidth-controlled, would be a good thing. However
> it would have side effects, such as interfering deeply with the server page
> cache, which may or may not be desirable.

In what is that different from VACUUM or a sequential scan? It is
possible to use buffer ring replacement strategies in such cases using
the normal clock-sweep algorithm, so that scanning a range of pages
does not really impact Postgres shared buffer cache.
--
Michael

Bonjour Michaël,

>> I agree that having a server function (extension?) to do a full checksum
>> verification, possibly bandwidth-controlled, would be a good thing. However
>> it would have side effects, such as interfering deeply with the server page
>> cache, which may or may not be desirable.
>
> In what is that different from VACUUM or a sequential scan?

Scrubbing would read all files, not only relation data? I'm unsure about
what does VACUUM, but it is probably pretty similar.

> It is possible to use buffer ring replacement strategies in such cases
> using the normal clock-sweep algorithm, so that scanning a range of
> pages does not really impact Postgres shared buffer cache.

Good! I did not know that there was an existing strategy to avoid filling
the cache.

--
Fabien.

On Mon, Mar 4, 2019, 04:10 Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Sun, Mar 03, 2019 at 07:58:26AM +0100, Fabien COELHO wrote:
> > I agree that having a server function (extension?) to do a full checksum
> > verification, possibly bandwidth-controlled, would be a good thing.
> However
> > it would have side effects, such as interfering deeply with the server
> page
> > cache, which may or may not be desirable.
>
> In what is that different from VACUUM or a sequential scan? It is
> possible to use buffer ring replacement strategies in such cases using
> the normal clock-sweep algorithm, so that scanning a range of pages
> does not really impact Postgres shared buffer cache.
>

Yeah, I wouldn't worry too much about the effect on the postgres cache when
that is done. It could of course have a much worse impact on the os cache
or on the "smart" (aka dumb) storage system cache. But that effect will be
there just as much with a separate tool.

/Magnus

On 3/4/19 4:09 AM, Michael Paquier wrote:
> On Sun, Mar 03, 2019 at 07:58:26AM +0100, Fabien COELHO wrote:
>> I agree that having a server function (extension?) to do a full checksum
>> verification, possibly bandwidth-controlled, would be a good thing. However
>> it would have side effects, such as interfering deeply with the server page
>> cache, which may or may not be desirable.
>
> In what is that different from VACUUM or a sequential scan? It is
> possible to use buffer ring replacement strategies in such cases using
> the normal clock-sweep algorithm, so that scanning a range of pages
> does not really impact Postgres shared buffer cache.
> --

But Fabien was talking about page cache, not shared buffers. And we
can't use custom ring buffer there. OTOH I don't see why accessing the
file through SQL function would behave any differently than direct
access (i.e. what the tool does now).

regards

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

On 3/4/19 2:00 AM, Michael Paquier wrote:
> On Sun, Mar 03, 2019 at 03:12:51AM +0100, Tomas Vondra wrote:
>> You and Andres may be right that trying to verify checksums online
>> without close interaction with the server is ultimately futile (or at
>> least overly complex). But I'm not sure those issues (torn pages and
>> partial reads) are very good arguments, considering basebackup has to
>> deal with them too. Not sure.
>
> FWIW, I don't think that the backend is right in its way of checking
> checksums the way it does currently either with warnings and a limited
> set of failures generated. I raised concerns about that unfortunately
> after 11 has been GA'ed, which was too late, so this time, for this
> patch, I prefer raising them before the fact and I'd rather not spread
> this kind of methodology around the core code more and more.

I still don't understand what issue you see in how basebackup verifies
checksums. Can you point me to the explanation you've sent after 11 was
released?

> I work a lot with virtualization, and I have seen ESX hanging around
> I/O requests from time to time depending on the environment used
> (which is actually wrong, anyway, but a lot of tests happen on a
> daily basis on the stuff I work on). What's presented on this thread
> is *never* going to be 100% safe, and would generate false positives
> which can be confusing for the user. This is not a good sign.

So you have a workload/configuration that actually results in data
corruption yet we fail to detect that? Or we generate false positives?
Or what do you mean by "100% safe" here?

regards

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

On Mon, Mar 4, 2019 at 3:02 PM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

>
>
> On 3/4/19 4:09 AM, Michael Paquier wrote:
> > On Sun, Mar 03, 2019 at 07:58:26AM +0100, Fabien COELHO wrote:
> >> I agree that having a server function (extension?) to do a full checksum
> >> verification, possibly bandwidth-controlled, would be a good thing.
> However
> >> it would have side effects, such as interfering deeply with the server
> page
> >> cache, which may or may not be desirable.
> >
> > In what is that different from VACUUM or a sequential scan? It is
> > possible to use buffer ring replacement strategies in such cases using
> > the normal clock-sweep algorithm, so that scanning a range of pages
> > does not really impact Postgres shared buffer cache.
> > --
>
> But Fabien was talking about page cache, not shared buffers. And we
> can't use custom ring buffer there. OTOH I don't see why accessing the
> file through SQL function would behave any differently than direct
> access (i.e. what the tool does now).
>

It shouldn't.

One other thought that I had around this though, which if it's been covered
before and I missed it, please disregard :)

The *online* version of the tool is very similar to running pg_basebackup
to /dev/null, is it not? Except it doesn't set the cluster to backup mode.
Perhaps what we really want is a simpler way to do *that*. That wouldn't
necessarily make it a SQL callable function, but it would be a CLI tool
that would call a command on a walsender for example.

(We'd of course still need the standalone tool for offline checks)

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

On Mon, Mar 04, 2019 at 03:08:09PM +0100, Tomas Vondra wrote:
> I still don't understand what issue you see in how basebackup verifies
> checksums. Can you point me to the explanation you've sent after 11 was
> released?

The history is mostly on this thread:
/message-id/20181020044248.GD2553@paquier.xyz

> So you have a workload/configuration that actually results in data
> corruption yet we fail to detect that? Or we generate false positives?
> Or what do you mean by "100% safe" here?

What's proposed on this thread could generate false positives. Checks
which have deterministic properties and clean failure handling are
reliable when it comes to reports.
--
Michael

On 3/5/19 4:12 AM, Michael Paquier wrote:
> On Mon, Mar 04, 2019 at 03:08:09PM +0100, Tomas Vondra wrote:
>> I still don't understand what issue you see in how basebackup verifies
>> checksums. Can you point me to the explanation you've sent after 11 was
>> released?
>
> The history is mostly on this thread:
> /message-id/20181020044248.GD2553@paquier.xyz
>

Thanks, will look.

Based on quickly skimming that thread the main issue seems to be
deciding which files in the data directory are expected to have
checksums. Which is a valid issue, of course, but I was expecting
something about partial read/writes etc.

>> So you have a workload/configuration that actually results in data
>> corruption yet we fail to detect that? Or we generate false positives?
>> Or what do you mean by "100% safe" here?
>
> What's proposed on this thread could generate false positives. Checks
> which have deterministic properties and clean failure handling are
> reliable when it comes to reports.

My understanding is that:

(a) The checksum verification should not generate false positives (same
as for basebackup).

(b) The partial reads do emit warnings, which might be considered false
positives I guess. Which is why I'm arguing for changing it to do the
same thing basebackup does, i.e. ignore this.

regards

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

On Tue, Mar 05, 2019 at 02:08:03PM +0100, Tomas Vondra wrote:
> Based on quickly skimming that thread the main issue seems to be
> deciding which files in the data directory are expected to have
> checksums. Which is a valid issue, of course, but I was expecting
> something about partial read/writes etc.

I remember complaining about partial write handling as well for the
base backup checks... There should be an email about it on the list,
cannot find it now ;p

> My understanding is that:
>
> (a) The checksum verification should not generate false positives (same
> as for basebackup).
>
> (b) The partial reads do emit warnings, which might be considered false
> positives I guess. Which is why I'm arguing for changing it to do the
> same thing basebackup does, i.e. ignore this.

Well, at least that's consistent... Argh, I really think that we
ought to make the failures reported harder because that's easier to
detect within a tool and some deployments set log_min_messages >
WARNING so checksum failures would just be lost. For base backups we
don't care much about that as files are just blindly copied so they
could have torn pages, which is fine as that's fixed at replay. Now
we are talking about a set of tools which could have reliable
detection mechanisms for those problems.
--
Michael

Greetings,

On Tue, Mar 5, 2019 at 18:36 Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Tue, Mar 05, 2019 at 02:08:03PM +0100, Tomas Vondra wrote:
> > Based on quickly skimming that thread the main issue seems to be
> > deciding which files in the data directory are expected to have
> > checksums. Which is a valid issue, of course, but I was expecting
> > something about partial read/writes etc.
>
> I remember complaining about partial write handling as well for the
> base backup checks... There should be an email about it on the list,
> cannot find it now ;p
>
> > My understanding is that:
> >
> > (a) The checksum verification should not generate false positives (same
> > as for basebackup).
> >
> > (b) The partial reads do emit warnings, which might be considered false
> > positives I guess. Which is why I'm arguing for changing it to do the
> > same thing basebackup does, i.e. ignore this.
>
> Well, at least that's consistent... Argh, I really think that we
> ought to make the failures reported harder because that's easier to
> detect within a tool and some deployments set log_min_messages >
> WARNING so checksum failures would just be lost. For base backups we
> don't care much about that as files are just blindly copied so they
> could have torn pages, which is fine as that's fixed at replay. Now
> we are talking about a set of tools which could have reliable
> detection mechanisms for those problems.

I’m traveling but will try to comment more in the coming days but in
general I agree with Tomas on these items. Also, pg_basebackup has to
handle torn pages when it comes to checksums just like the verify tool
does, and having them be consistent (along with external tools) would
really be for the best, imv. I still feel like a retry of a short read
(try reading more to get the whole page..) would be alright and reading
until we hit eof and then moving on. I’m not sure it’s possible but I do
worry a bit that we might get a short read from a network file system or
something that isn’t actually at eof and then we would skip a significant
remaining portion of the file... another thought might be to stat the
file after we have opened it to see it’s length...

Just a few thoughts since I’m on my phone. Will try to write up something
more in a day or two.

Thanks!

Stephen

On Sat, Mar 2, 2019 at 4:38 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> FWIW I don't think this qualifies as torn page - i.e. it's not a full
> read with a mix of old and new data. This is partial write, most likely
> because we read the blocks one by one, and when we hit the last page
> while the table is being extended, we may only see the fist 4kB. And if
> we retry very fast, we may still see only the first 4kB.

I see the distinction you're making, and you're right. The problem
is, whether in this case or whether for a real torn page, we don't
seem to have a way to distinguish between a state that occurs
transiently due to lack of synchronization and a situation that is
permanent and means that we have corruption. And that worries me,
because it means we'll either report bogus complaints that will scare
easily-panicked users (and anybody who is running this tool has a good
chance of being in the "easily-panicked" category ...), or else we'll
skip reporting real problems. Neither is good.

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

On Sat, Mar 2, 2019 at 5:45 AM Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
> Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> > On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> > <michael(dot)banck(at)credativ(dot)de> wrote:
> > > I have added a retry for this as well now, without a pg_sleep() as well.
> > > This catches around 80% of the half-reads, but a few slip through. At
> > > that point we bail out with exit(1), and the user can try again, which I
> > > think is fine?
> >
> > Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> > robust at all.
>
> The chance that pg_verify_checksums hits a torn page (at least in my
> tests, see below) is already pretty low, a couple of times per 1000
> runs. Maybe 4 out 5 times, the page is read fine on retry and we march
> on. Otherwise, we now just issue a warning and skip the file (or so was
> the idea, see below), do you think that is not acceptable?

Yeah. Consider a paranoid customer with 100 clusters who runs this
every day on every cluster. They're going to see failures every day
or three and go ballistic.

I suspect that better retry logic might help here. I mean, I would
guess that 10 retries at 1 second intervals or something of that sort
would be enough to virtually eliminate false positives while still
allowing us to report persistent -- and thus real -- problems. But if
even that is going to produce false positives with any measurable
probability different from zero, then I think we have a problem,
because I neither like a verification tool that ignores possible signs
of trouble nor one that "cries wolf" when things are fine.

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

On 2019-03-06 12:33:49 -0500, Robert Haas wrote:
> On Sat, Mar 2, 2019 at 5:45 AM Michael Banck <michael(dot)banck(at)credativ(dot)de> wrote:
> > Am Freitag, den 01.03.2019, 18:03 -0500 schrieb Robert Haas:
> > > On Tue, Sep 18, 2018 at 10:37 AM Michael Banck
> > > <michael(dot)banck(at)credativ(dot)de> wrote:
> > > > I have added a retry for this as well now, without a pg_sleep() as well.
> > > > This catches around 80% of the half-reads, but a few slip through. At
> > > > that point we bail out with exit(1), and the user can try again, which I
> > > > think is fine?
> > >
> > > Maybe I'm confused here, but catching 80% of torn pages doesn't sound
> > > robust at all.
> >
> > The chance that pg_verify_checksums hits a torn page (at least in my
> > tests, see below) is already pretty low, a couple of times per 1000
> > runs. Maybe 4 out 5 times, the page is read fine on retry and we march
> > on. Otherwise, we now just issue a warning and skip the file (or so was
> > the idea, see below), do you think that is not acceptable?
>
> Yeah. Consider a paranoid customer with 100 clusters who runs this
> every day on every cluster. They're going to see failures every day
> or three and go ballistic.

+1

> I suspect that better retry logic might help here. I mean, I would
> guess that 10 retries at 1 second intervals or something of that sort
> would be enough to virtually eliminate false positives while still
> allowing us to report persistent -- and thus real -- problems. But if
> even that is going to produce false positives with any measurable
> probability different from zero, then I think we have a problem,
> because I neither like a verification tool that ignores possible signs
> of trouble nor one that "cries wolf" when things are fine.

To me the right way seems to be to IO lock the page via PG after such a
failure, and then retry. Which should be relatively easily doable for
the basebackup case, but obviously harder for the pg_verify_checksums
case.

Greetings,

Andres Freund