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 |
Date: | 2020-04-06 19:59:15 |
Message-ID: | 26184.1586203155@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-Fix-checksum-verification-in-base-backups-for-random_V4.patch | text/x-diff | 7.6 KB |
From | Date | Subject | |
---|---|---|---|
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) |