From: | Michael Banck <michael(dot)banck(at)credativ(dot)de> |
---|---|
To: | Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Online verification of checksums |
Date: | 2020-03-12 06:32:12 |
Message-ID: | 28133de5f2acb9e2d16ba31372e80828f6262b92.camel@credativ.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)banck(at)credativ(dot)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
Attachment | Content-Type | Size |
---|---|---|
0001-Fix-checksum-verification-in-base-backups-for-random_V3.patch | text/x-patch | 12.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tobias Stadler | 2020-03-12 06:45:30 | Re: User and Logical Replication |
Previous Message | Julien Rouhaud | 2020-03-12 06:31:09 | Re: Planning counters in pg_stat_statements (using pgss_store) |