Re: [Patch] Checksums for SLRU files

From: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
To: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
Cc: Ivan Kartyshov <i(dot)kartyshov(at)postgrespro(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [Patch] Checksums for SLRU files
Date: 2018-01-02 22:21:34
Views: Raw Message | Whole Thread | Download mbox
Lists: pgsql-hackers

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
> 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,
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:
The Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2018-01-02 22:23:54 heads up: Fix for intel hardware bug will lead to performance regressions
Previous Message Nikita Glukhov 2018-01-02 22:04:49 Re: [HACKERS] SQL/JSON in PostgreSQL