Subject: Re: pgsql: Validate page level checksums in base backups
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Magnus Hagander <magnus@hagander.net>,
 Michael Banck <michael.banck@credativ.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Tom Lane <tgl@sss.pgh.pa.us>,
 Andres Freund <andres@anarazel.de>, David Steele <david@pgmasters.net>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20180404121930.GE20852@nighthawk.caipicrew.dd-dns.de>
 <20180404134732.tsxx2osvooolxy24@alvherre.pgsql>
 <CABUevEwrCF7TzAyD=4WPfTyzH+5xaFYf+-=ATW58XLi7b4dURA@mail.gmail.com>
 <20180404182247.GG20852@nighthawk.caipicrew.dd-dns.de>
 <CABUevExp8BOo3ZjEXM5xkz5P0zrjZT4_eM7QM4DLyPorDVUQUQ@mail.gmail.com>
 <20180405124113.GH20852@nighthawk.caipicrew.dd-dns.de>
 <CABUevEzZ55qgLVtMxW=hXhzjHV3oU-gChpeKtufyf_siL-rBWg@mail.gmail.com>
 <f23e92ec-118d-e6ea-0c81-876ad62a588a@2ndquadrant.com>
Message-ID: <500ab687-d588-020a-0e23-bfec853aac9c@2ndquadrant.com>
Date: Tue, 10 Apr 2018 23:35:21 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <f23e92ec-118d-e6ea-0c81-876ad62a588a@2ndquadrant.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 04/10/2018 11:24 PM, Tomas Vondra wrote:
> Hi,
> 
> I think there's a bug in sendFile(). We do check checksums on all pages
> that pass this LSN check:
> 
>     /*
>      * Only check pages which have not been modified since the
>      * start of the base backup. Otherwise, they might have been
>      * written only halfway and the checksum would not be valid.
>      * However, replaying WAL would reinstate the correct page in
>      * this case.
>      */
>     if (PageGetLSN(page) < startptr)
>     {
>         ...
>     }
> 
> Now, imagine the page is new, i.e. all-zeroes. That means the LSN is 0/0
> too, and we'll try to verify the checksum - but we actually do not set
> checksums on empty pages.
> 
> So I think it should be something like this:
> 
>     if ((!PageIsNew(page)) && (PageGetLSN(page) < startptr))
>     {
>         ...
>     }
> 
> It might be worth verifying that the page is actually all-zeroes (and
> not just with corrupted pd_upper value. Not sure it's worth it.
> 
> I've found this by fairly trivial stress testing - running pgbench and
> pg_basebackup in a loop. It was failing pretty reliably (~75% of runs).
> With the proposed change I see no further failures.
> 

BTW pg_verify_checksums needs the same fix.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services