> 31 дек. 2017 г., в 22:30, Ivan Kartyshov <i.kartyshov@postgrespro.ru> написал(а): > > Hello, I`d like to show my implementation of SLRU file protection with checksums. > ..... > I would like to hear your thoughts over my patch.
As far as I can see, the patch solves problem of hardware corruption in SLRU. This seems a valid concern. I've tried to understand your patch and few questions arose which I could not answer myself.
1. Why do you propose different GUC besides ignore_checksum_failure? What is scenario of it's use which is not covered by general GUC switch? 2. What is performance penalty of having this checksums?
Besides this, some things seems suspicious to me: 1. This comment seems excessive. I'd leave just first one first line. +/* + * GUC variable + * Set from backend: + * alter system set ignore_slru_checksum_failure = on/off; + * select pg_reload_conf(); + */ 2. Functions pg_checksum_slru_page() and pg_getchecksum_slru_page() operate with two bytes instead of uint16. This seems strange. 3. This line checksum = (pg_checksum_block(page, BLCKSZ) % 65535) + 1; Need to share comment with previous function (pg_checksum_page()). +1 was a tough thing for me to understand before looking around and reading those comments. 4. I could not understand purpose of this expression page[BLCKSZ - 1] & 0X00FF
Andrey, thank you for review.
Ivan, thank you for submitting this patch. I also have some notes on it from the first glance.
1. It seems that you need to define some macro for (BLCKSZ - CHKSUMSZ) in order to evade typing it multiple times.
2. You also didn't modify all the SLRU macros depending on useful size of page. You missed at least COMMIT_TS_XACTS_PER_PAGE, MULTIXACT_MEMBERGROUPS_PER_PAGE and SUBTRANS_XACTS_PER_PAGE.
3. pg_upgrade isn't considered. This patch should provide upgrading SLRUs to adopt changed useful size of page. That seems to be hardest patch of this patch to be written.