| From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
|---|---|
| To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Trying to break online checksums with LLMs |
| Date: | 2026-09-10 20:33:21 |
| Message-ID: | CA+TgmoaOCdjAjr240e_+xoqQCRmLC9MFn3kZOu-7j8w8HBrd8g@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, Sep 8, 2026 at 5:38 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> Noah kindly ran Claude- and GPT-automated reviews on the online checksums
> feature with the fix from the open item applied. Since he has been very
> successful in identifying previously unknown issues in other features I was
> interested in seeing what could shake loose. The full reports are attached,
> with an executive summary of the identified issues and the fixes below. It's
> mostly documentation or corner cases from re-starting operations within tight
> windows, none of the issues would lead to incorrect results or false negatives
> or positives in page verification. Fixes are in separate patches for easier
> review, for a commit I would group the doc ones together.
Nice.
Patch 0003's commit message writes "th" where "the" is intended.
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.
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.
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Masahiko Sawada | 2026-09-10 20:34:27 | Re: Misplaced comment in snapbuild.c |
| Previous Message | Ilia Evdokimov | 2026-09-10 20:20:05 | Re: Fix var_eq_const: sum selectivity of all matching MCV entries instead of stopping at first match |