Re: [Patch] Checksums for SLRU files

On Mon, Jan 1, 2018 at 9:19 PM, Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:

> > 31 дек. 2017 г., в 22:30, Ivan Kartyshov <i(dot)kartyshov(at)postgrespro(dot)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