Re: [PATCH] Verify Checksums during Basebackups

Greetings,

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> On Fri, Mar 30, 2018 at 5:35 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>
> > On 3/24/18 10:32 AM, Michael Banck wrote:
> > > Am Freitag, den 23.03.2018, 17:43 +0100 schrieb Michael Banck:
> > >> Am Freitag, den 23.03.2018, 10:54 -0400 schrieb David Steele:
> > >>> In my experience actual block errors are relatively rare, so there
> > >>> aren't likely to be more than a few in a file. More common are
> > >>> overwritten or transposed files, rogue files, etc. These produce a lot
> > >>> of output.
> > >>>
> > >>> Maybe stop after five?
> > >
> > > The attached patch does that, and outputs the total number of
> > > verification failures of that file after it got sent.
> > >
> > >> I'm on board with this, but I have the feeling that this is not a very
> > >> common pattern in Postgres, or might not be project style at all. I
> > >> can't remember even seen an error message like that.
> > >>
> > >> Anybody know whether we're doing this in a similar fashion elsewhere?
> > >
> > > I tried to have look around and couldn't find any examples, so I'm not
> > > sure that patch should go in. On the other hand, we abort on checksum
> > > failures usually (in pg_dump e.g.), so limiting the number of warnings
> > > does makes sense.
> > >
> > > I guess we need to see what others think.
> >
> > Well, at this point I would say silence more or less gives consent.
> >
> > Can you provide a rebased patch with the validation retry and warning
> > limiting logic added? I would like to take another pass through it but I
> > think this is getting close.
>
> I was meaning to mention it, but ran out of cycles.
>
> I think this is the right way to do it, except the 5 should be a #define
> and not an inline hardcoded value :) We could argue whether it should be "5
> total" or "5 per file". When I read the emails I thought it was going to be
> 5 total, but I see the implementation does 5 / file. In a super-damanged
> system that will still lead to horrible amounts of logging, but I think
> maybe if your system is in that bad shape, then it's a lost cause anyway.

5/file seems reasonable to me as well.

> I also think the "total number of checksum errors" should be logged if
> they're >0, not >5. And I think *that* one should be logged at the end of
> the entire process, not per file. That'd be the kind of output that would
> be the most interesting, I think (e.g. if I have it spread out with 1 block
> each across 4 files, I want that logged at the end because it's easy to
> otherwise miss one or two of them that may have happened a long time apart).

I definitely like having a total # of checksum errors included at the
end, if there are any at all. When someone is looking to see why the
process returned a non-zero exit code, they're likely to start looking
at the end of the log, so having that easily available and clear as to
why the backup failed is definitely valuable.

> I don't think we have a good comparison elsewhere, and that is as David
> mention because other codepaths fail hard when they run into something like
> that. And we explicitly want to *not* fail hard, per previous discussion.

Agreed.

Thanks!

Stephen