pg_stat_database.checksum_failures misses single-page failures in backups
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253454psql -h localhost -U postgresBuilt from patchset v7 (message #7), August 18, 2026 at 07:04 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253454_7 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253454_7 && git checkout t253454_7Patchset v7 (message #7) is on t253454_7
Hello!
This issue was reported by Yilin Zhang in another backup related thread[1]/messages/by-id/34b127c.6e5d.1a00f5540f9.Coremail.jiezhilove@126.com
When basebackup finds exactly one corrupted page in a file, the backup
fails as expected with a per-page warning and then an error. However,
it doesn't update the statistics, because that update is currently
inside the per-file summary for 2 or more checksum failures.
This issue seems to exists since the introduction of the feature (6b9e875f728).
The attached patch moves the statistics report into its own block.
[1]: /messages/by-id/34b127c.6e5d.1a00f5540f9.Coremail.jiezhilove@126.com
Hi,
On Mon, 17 Aug 2026 at 16:11, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
This issue was reported by Yilin Zhang in another backup related thread[1]
When basebackup finds exactly one corrupted page in a file, the backup
fails as expected with a per-page warning and then an error. However,
it doesn't update the statistics, because that update is currently
inside the per-file summary for 2 or more checksum failures.This issue seems to exists since the introduction of the feature (6b9e875f728).
The attached patch moves the statistics report into its own block.
I was reading the related thread [1]/messages/by-id/flat/20180228180856.GE13784@nighthawk.caipicrew.dd-dns.de and I think you are right, it is
an oversight. Your analysis and fix look correct to me.
[1]: /messages/by-id/flat/20180228180856.GE13784@nighthawk.caipicrew.dd-dns.de
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Mon, Aug 17, 2026 at 05:32:09PM +0300, Nazir Bilal Yavuz wrote:
I was reading the related thread [1] and I think you are right, it is
an oversight. Your analysis and fix look correct to me.
Fun one. On corruption I feel that it would be unlikely to see only 1
failure after a large scan. That's still wrong. :)
if (checksum_failures > 1)
- {
ereport(WARNING,
(errmsg_plural("file \"%s\" has a total of %d checksum verification failure",
"file \"%s\" has a total of %d checksum verification failures",
checksum_failures,
readfilename, checksum_failures)));
+ if (checksum_failures > 0)
+ {
pgstat_prepare_report_checksum_failure(dboid);
pgstat_report_checksum_failures_in_db(dboid, checksum_failures);
}
While the report is right, the resulting patch looks incorrect to me.
It seems to me that we should also report the warning if we have
checksum_failures == 1. errmsg_plural() would use the non-plural
message in this case. So it looks like we should just switch
"checksum_failures > 1" to "checksum_failures > 0".
Adding a test case seems like a good idea. Your proposal is cheap.
Cheap is good.
--
Michael
While the report is right, the resulting patch looks incorrect to me.
It seems to me that we should also report the warning if we have
checksum_failures == 1
I am not sure, maybe. The original commit 6b9e875f728 only had the plural form in it. The errmsg_plural was added as part of a message style sweep commit in 887248e97e2d.
To me it seems like that the original intent was to only print out the summaries (both this and the similar total_checksum_failures > 1 check in the same file) if we have more than 1 failure. If we only have 1, it only prints out the specific info about that failure.
If you think that's better I can simply change the 1 to 0 in this if, and then for consistency also do the same change in the other if, but that way we also change what warnings we print out in this case.
On Mon, Aug 17, 2026 at 06:01:01PM -0500, Zsolt Parragi wrote:
To me it seems like that the original intent was to only print out the
summaries (both this and the similar total_checksum_failures > 1 check
in the same file) if we have more than 1 failure. If we only have 1,
it only prints out the specific info about that failure.If you think that's better I can simply change the 1 to 0 in this if,
and then for consistency also do the same change in the other if, but
that way we also change what warnings we print out in this case.
Users tend to ignore entirely WARNINGs in my experience, but it also
sounds to me that it is better to inform if the stats view has been
incremented at all even if only one page was iffy, starting at
strictly more than 0, not strictly more than 1 because the information
is still useful.
--
Michael
On Tue, Aug 18, 2026 at 08:11:04AM +0900, Michael Paquier wrote:
Users tend to ignore entirely WARNINGs in my experience, but it also
sounds to me that it is better to inform if the stats view has been
incremented at all even if only one page was iffy, starting at
strictly more than 0, not strictly more than 1 because the information
is still useful.
In order to close the loop, I have been doing some archeology.
I can track the original code change in the v5 posted here, which has
been using a (> 1) for the sake of the plural message, at least that
feels like the author's intention:
/messages/by-id/20180331125404.GA20852@nighthawk.caipicrew.dd-dns.de
I would still just adjust the one-off in the if condition at the end.
Reporting also a WARNING even if we have 1 checksum failure feels like
the right thing to do.
--
Michael
I can track the original code change in the v5 posted here, which has
been using a (> 1) for the sake of the plural message, at least that
feels like the author's intention:
I read the intention there differently, as errmsg_plural has been
available since 2009.
I would still just adjust the one-off in the if condition at the end.
Reporting also a WARNING even if we have 1 checksum failure feels like
the right thing to do.
I did that in v2, I also adjusted the total warning I mentioned earlier.