|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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
> > .....
> > 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
> 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.
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|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|