| From: | Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Cc: | Michael Banck <michael(dot)banck(at)credativ(dot)de> |
| Subject: | Re: Online verification of checksums |
| Date: | 2020-02-27 10:57:09 |
| Message-ID: | 158280102962.21707.6408582526895921673.pgcf@coridan.postgresql.org |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
- Remove the empty line:
+ * would throw an assertion failure. Consider this a
+ * checksum failure.
+ */
+
+ checksum_failures++;
- Code needs to run through pgindent.
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.
Regards,
--Asif
The new status of this patch is: Waiting on Author
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Fujii Masao | 2020-02-27 11:04:41 | Re: Crash by targetted recovery |
| Previous Message | Fujii Masao | 2020-02-27 10:13:54 | Re: Improve handling of parameter differences in physical replication |