Re: Substituting Checksum Algorithm (was: Enabling Checksums)

On 2013-04-30 11:55:29 +0100, Simon Riggs wrote:
> ISTM that we also need this patch to put memory barriers in place
> otherwise the code might be rearranged.
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

> --- a/src/backend/storage/page/bufpage.c
> +++ b/src/backend/storage/page/bufpage.c
> @@ -960,11 +960,14 @@ PageCalcChecksum16(Page page, BlockNumber blkno)
> * Save pd_checksum and set it to zero, so that the checksum calculation
> * isn't affected by the checksum stored on the page. We do this to
> * allow optimization of the checksum calculation on the whole block
> - * in one go.
> + * in one go. Memory barriers are required to avoid rearrangement here.
> */
> save_checksum = phdr->pd_checksum;
> + pg_memory_barrier();
> phdr->pd_checksum = 0;
> + pg_memory_barrier();
> checksum = checksum_block(page, BLCKSZ);
> + pg_memory_barrier();
> phdr->pd_checksum = save_checksum;
>
> /* mix in the block number to detect transposed pages */

Why? I am not sure which rearrangement you're fearing? In all cases
where there is danger of concurrent write access to the page we should
already be working on a copy?
Also, if we need a memory barrier I can only see a point in the 2nd
one. The first and third shouldn't ever be able to change anything since
we are only writing to local memory?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services