Re: Online checksums verification in the backend

On Fri, Oct 23, 2020 at 3:28 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Oct 19, 2020 at 04:52:48PM +0900, Michael Paquier wrote:
> > No issues with reporting the block number and the fork type in the SRF
> > at least of course as this is helpful to detect the position of the
> > broken blocks. For the checksum found in the header and the one
> > calculated (named expected in the patch), I am less sure how to put a
> > clear definition on top of that but we could always consider that
> > later and extend the SRF as needed. Once the user knows that both do
> > not match, he/she knows that there is a problem.
>
> So, I have reviewed your patch set, and heavily reworked the logic to
> be more consistent on many thinks, resulting in a largely simplified
> patch without sacrificing its usefulness:

Thanks!

> - Removal of the dependency with checksums for this feature. While
> simplifying the code, I have noticed that this feature can also be
> beneficial for clusters that do not have have data checksums, as
> PageIsVerified() is perfectly able to run some page header checks and
> the zero-page case. That's of course less useful than having the
> checksums, but there is no need to add a dependency here. The file
> for the SQL functions is renamed from checksumfuncs.c to pagefuncs.c.

I agree. However I'm assuming that this refactor is relying on the
not yet committed patch (in the nearby thread dealing with base backup
checksums check) to also refactor PageIsVerified? As all the logic
you removed was done to avoid spamming a lot of warnings when calling
the function.

> - The function is changed to return no tuples if the relkind is not
> supported, and the same applies for temporary relations. That's more
> consistent with other system functions like the ones in charge of
> partition information, and this makes full scans of pg_class much
> easier to work with. Temporary tables were not handled correctly
> anyway as these are in local buffers, but the use case of this
> function in this case is not really obvious to me.

Agreed

> - Having the forknum in the SRF is kind of confusing, as the user
> would need to map a number with the physical on-disk name. Instead, I
> have made the choice to return the *path* of the corrupted file with a
> block number. This way, an operator can know immediately where a
> problem comes from. The patch does not append the segment number, and
> I am not sure if we should actually do that, but adding it is
> straight-forward as we have the block number. There is a dependency
> with table AMs here as well, as this goes down to fd.c, explaining why
> I have not added it and just.

That's a clear improvement, thanks!

> - I really don't know what kind of default ACL should apply for such
> functions, but I am sure that SCAN_TABLES is not what we are looking
> for here, and there is nothing preventing us from having a safe
> default from the start of times, so I moved the function to be
> superuser-only by default, and GRANT can be used to allow its
> execution to other roles. We could relax that in the future, of
> course, this can be discussed separately.

I don't have a strong opinion here, SCAN_TABLES was maybe not ideal.
No objections.

> - The WARNING report for each block found as corrupted gains an error
> context, as a result of a switch to PageIsVerified(), giving a user
> all the information needed in the logs on top of the result in the
> SRF. That's useful as well if combined with CHECK_FOR_INTERRUPTS(),
> and I got to wonder if we should have some progress report for this
> stuff, though that's a separate discussion.

Mmm, is it really an improvement to report warnings during this
function execution? Note also that PageIsVerified as-is won't report
a warning if a page is found as PageIsNew() but isn't actually all
zero, while still being reported as corrupted by the SRF.

Have you also considered that it's possible to execute
pg_relation_check_pages with ignore_checksum_failure = on? That's
evidently a bad idea, but doing so would report some of the data
corruption as warnings while still not reporting anything in the SRF.

Having some progress report would be nice to have, but +1 to have a
separate discussion for that.

> - The function is renamed to something less generic,
> pg_relation_check_pages(), and I have reduced the number of functions
> from two to one, where the user can specify the fork name with a new
> option. The default of NULL means that all the forks of a relation
> are checked.

Ok.

> - The TAP tests are rather bulky. I have moved all the non-corruption
> test cases into a new SQL test file. That's useful for people willing
> to do some basic sanity checks with a non-default table AM. At least
> it provides a minimum coverage.

Agreed