> On 10 Sep 2026, at 22:33, Robert Haas <[email protected]> wrote:
Thanks for reviewing! > Patch 0003's commit message writes "th" where "the" is intended. Ugh, will fix. > Regarding 0005, Claude is suggesting to me that this might be the > wrong fix. It says (I think correctly) that the real problem is the > code at the end of launcher_exit, which is willing to set > DataChecksumState->launcher_running = false even if our process was > not the one that set that value to true. Unless our local copy of > launcher_running is true, we have no business calling > SetDataChecksumsOff() or setting DataChecksumState->launcher_running = > false. If you fix that, then perhaps it's OK for > DataChecksumsWorkerLauncherMain to stay as it is: the problem there is > that after DataChecksumsWorkerLauncherMain sets launcher_running back > to false, launcher_exit() can still run and decide to mess with shared > state. Interesting, I will have a look at that tomorrow morning after coffee. > Another small problem that Claude found: Within > DataChecksumsWorkerLauncherMain, if an abort is requested, we will set > abort_requested = true and goto done. From there we can goto again. > Now we're starting over, but abort_requested is still true, because > nothing resets it. If the user starts to enable checksums, then > changes their mind and starts to disable them, then changes their mind > again and starts to enable them, the second enable will get confused > and not complete properly. I think this is a one-line fix. Right, in this case the second enable will not proceed and checksums will end up disabled. Will have a look. > But much more seriously, Claude also noticed this: In general, if > PostgreSQL creates a relation and then crashes before commit, the > relation files are left on disk and not removed, but there's no > catalog entry pointing to them any more. If checksums are enabled > using pg_checksums, such files are updated to have checksums as well. > But this feature enables checksums by working through the catalog, so > it won't update those files. That means that if, at any point in the > history of the cluster, there's ever been a crash that left a relation > file behind on disk, and then you use this feature to enable > checksums, and then after that you take a base backup, checksum > verification will fail, and thus the backup will fail. I don't quite > see how we can actually fix this, and it seems like a serious problem. I have a feeling this had been brought up in one of all the postcommit threads, but I fail to find it now (perhaps it was a basebackup and not checksums thread?). The gist of that discussion was that there is somewhere between very little add nothing we can do to protect against orphaned/external files in the datadir when using the catalog as the main source of truth for what we expect it to contain. I wonder if the best we can do is to document exactly how to verify the state of the page when getting a backup page verification failure? -- Daniel Gustafsson
