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