On Fri, Apr 03, 2020 at 11:39:11AM +0200, Julien Rouhaud wrote:
> On Fri, Apr 03, 2020 at 12:24:50PM +0900, Masahiko Sawada wrote:
> >
> > check_relation_fork() seems to quite depends on pg_check_relation()
> > because the returned tuplestore is specified by pg_check_relation().
> > It's just an idea but to improve reusability, how about moving
> > check_relation_fork() to checksumfunc.c? That is, in checksumfuncs.c
> > while iterating all blocks we call a new function in checksum.c, say
> > check_one_block() function, which has the following part and is
> > responsible for getting, checking the specified block and returning a
> > boolean indicating whether the block has corruption or not, along with
> > chk_found and chk_expected:
> >
> > /*
> > * To avoid too much overhead, the buffer will be first read without
> > * the locks that would guarantee the lack of false positive, as such
> > * events should be quite rare.
> > */
> > Retry:
> > if (!check_get_buffer(relation, forknum, blkno, buffer, force_lock,
> > &found_in_sb))
> > continue;
> >
> > if (check_buffer(buffer, blkno, &chk_expected, &chk_found))
> > continue;
> >
> > /*
> > * If we get a failure and the buffer wasn't found in shared buffers,
> > * reread the buffer with suitable lock to avoid false positive. See
> > * check_get_buffer for more details.
> > */
> > if (!found_in_sb && !force_lock)
> > {
> > force_lock = true;
> > goto Retry;
> > }
> >
> > A new function in checksumfuncs.c or pg_check_relation will be
> > responsible for storing the result to the tuplestore. That way,
> > check_one_block() will be useful for other use when we want to check
> > if the particular block has corruption with low overhead.
>
>
> Yes, I agree that passing the tuplestore isn't an ideal approach and some
> refactoring should probably happen. One thing is that this wouldn't be
> "check_one_block()" but "check_one_block_on_disk()" (which could also be from
> the OS cache). I'm not sure how useful it's in itself. It also raises some
> concerns about the throttling. I didn't change that for now, but I hope
> there'll be some other feedback about it.
>
I had some time this morning, so I did the suggested refactoring as it seems
like a way cleaner interface. I also kept the suggested check_one_block().