Re: [Patch] Checksums for SLRU files - Mailing list pgsql-hackers

From Andrey Borodin
Subject Re: [Patch] Checksums for SLRU files
Date
Msg-id FE81F235-AEEE-480A-BFFF-E9B84D26C4A7@yandex-team.ru
Whole thread Raw
In response to [Patch] Checksums for SLRU files  (Ivan Kartyshov <i.kartyshov@postgrespro.ru>)
Responses Re: [Patch] Checksums for SLRU files
List pgsql-hackers
Hi, Ivan!
> 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
bygeneral 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
seemsstrange. 
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
lookingaround and reading those comments. 
4. I could not understand purpose of this expression
page[BLCKSZ - 1] & 0X00FF

Happy New Year :) Keep up good work.

Best regards, Andrey Borodin.

pgsql-hackers by date:

Previous
From: Noah Misch
Date:
Subject: Re: Increasing timeout of poll_query_until for TAP tests
Next
From: Gavin Flower
Date:
Subject: Re: What does Time.MAX_VALUE actually represent?