Re: Online verification of checksums

Hi,

thanks for reviewing this patch!

Am Donnerstag, den 27.02.2020, 10:57 +0000 schrieb Asif Rehman:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
> 
> The patch applies cleanly and works as expected. Just a few minor observations:
> 
> - I would suggest refactoring PageIsZero function by getting rid of all_zeroes variable
> and simply returning false when a non-zero byte is found, rather than setting all_zeros
> variable to false and breaking the for loop. The function should simply return true at the
> end otherwise.


Good point, I have done so.

> - Remove the empty line:
> +                        * would throw an assertion failure.  Consider this a
> +                        * checksum failure.
> +                        */
> +
> +                       checksum_failures++;

Done

> - Code needs to run through pgindent.

Done.

> Also, I'd suggest to make "5" a define within the current file/function, perhaps 
> something like "MAX_CHECKSUM_FAILURES". You could move the second 
> warning outside the conditional statement as it appears in both "if" and "else" blocks.

Well, I think you have a valid point, but that would be a different (non
bug-fix) patch as this part is not changed by this patch, but code is at
most moved around, is it?

New version attached.


Best regards,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.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