On Mon, Jan 1, 2018 at 9:19 PM, Andrey Borodin <x4...@yandex-team.ru> wrote:
> > 31 дек. 2017 г., в 22:30, Ivan Kartyshov <i.kartys...@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. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
