|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|To:||Michael Banck <michael(dot)banck(at)credativ(dot)de>|
|Cc:||Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org|
|Subject:||Re: Online verification of checksums|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Michael Banck <michael(dot)banck(at)credativ(dot)de> writes:
> [ 0001-Fix-checksum-verification-in-base-backups-for-random_V3.patch ]
I noticed that the cfbot wasn't testing this because of a minor merge
conflict. I rebased it over that, and also readjusted things a little bit
to avoid unnecessarily reindenting existing code, in hopes of making the
patch easier to review. Doing that reveals that the patch actually
removes a chunk of code, namely a special case for EOF. Was that
intentional, or a result of a faulty merge earlier? It certainly isn't
mentioned in your proposed commit message.
Another thing that's bothering me is that the patch compares page LSN
against GetInsertRecPtr(); but that function says
* NOTE: The value *actually* returned is the position of the last full
* xlog page. It lags behind the real insert position by at most 1 page.
* For that, we don't need to scan through WAL insertion locks, and an
* approximation is enough for the current usage of this function.
I'm not convinced that an approximation is good enough here. It seems
like a page that's just now been updated could have an LSN beyond the
current XLOG page start, potentially leading to a false checksum
complaint. Maybe we could address that by adding one xlog page to
the GetInsertRecPtr result? Kind of a hack, but ...
regards, tom lane
|Next Message||Andrew Dunstan||2020-04-06 20:06:52||Re: backup manifests and contemporaneous buildfarm failures|
|Previous Message||Tomas Vondra||2020-04-06 19:57:22||Re: [PATCH] Incremental sort (was: PoC: Partial sort)|