On Fri, Oct 23, 2020 at 04:31:56PM +0800, Julien Rouhaud wrote:
> 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.
Yeah, it should use a refactored version, but I was as well in the
mood of looking at version based on what we have now on HEAD. Even if
I am not completely clear where the patch for page verification and
base backups will go, I was thinking as well to do the refactoring
introducing PageIsVerifiedExtended() first, before considering the
next steps for this thread. It seems to me that the path where we
generate no WARNINGs at all makes the whole experience more consistent
for the user with this function.
> 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.
Yep, joining the point of above to just have no WARNINGs at all.
> 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.
Yeah, I thought about that as well, but I did not see a strong
argument against preventing this behavior either, even if it sounds
a bit strange. We could always tune that later depending on the
feedback.
--
Michael