allow spread checkpoints when changing checksums online

Hi,

Here's a rebased patch, addressing the minor bitrot, and with some very
minor adjustments.

1) proper commit message

2) cleanup / rewording in the sgml docs

3) I've noticed the SGML docs don't have default values for the existing
parameters (cost_delay, cost_limit), so I added those. Maybe backpatch?

4) I've added the new 'fast' parameter to the DataChecksums module, but
mostly just for completeness. The TAP tests still use the default value,
so that it doesn't take longer.

I've spent some time thinking about the default value. For a while I
thought maybe it should default to 'false' (in which case we use spread
checkpoints, which is the less disruptive option).

But that's actually futile, because the cost_delay still defaults to 0,
which means the "data page rewrite" phase won't be paced. It seems
surprising the first phase would not be throttled by default, while the
checkpoints are ...

So I ended up setting the 'fast=true' default.

This is not a very complicated patch, and it's been part of the old
patch series for quite a while. So unless there's some objections I'll
get this committed in a couple days.

regards

--
Tomas Vondra

On 6 Jul 2026, at 13:43, Tomas Vondra <tomas@vondra.me> wrote:

Here's a rebased patch, addressing the minor bitrot, and with some very
minor adjustments.

Thanks for picking up this!

3) I've noticed the SGML docs don't have default values for the existing
parameters (cost_delay, cost_limit), so I added those. Maybe backpatch?

+1, that sounds like a good idea.

4) I've added the new 'fast' parameter to the DataChecksums module, but
mostly just for completeness. The TAP tests still use the default value,
so that it doesn't take longer.

+ spread (throttled), which may increase the time needed to enable checksums,

I think we should stick to the "data checksums" terminology for consistency.

@@ -1676,6 +1686,7 @@ DataChecksumsWorkerMain(Datum arg)

DataChecksumState->cost_delay = DataChecksumState->launch_cost_delay;
DataChecksumState->cost_limit = DataChecksumState->launch_cost_limit;
+ DataChecksumState->fast_checkpoint = DataChecksumState->launch_fast_checkpoint;
}
else
costs_updated = false;

This update will only change the fast parameter if cost_limit or cost_delay
were changed, shouldn't the fast parameter also be checked for runtime update?

I've spent some time thinking about the default value. For a while I
thought maybe it should default to 'false' (in which case we use spread
checkpoints, which is the less disruptive option).

But that's actually futile, because the cost_delay still defaults to 0,
which means the "data page rewrite" phase won't be paced. It seems
surprising the first phase would not be throttled by default, while the
checkpoints are ...

So I ended up setting the 'fast=true' default.

I think fast=true is the right default.

This is not a very complicated patch, and it's been part of the old
patch series for quite a while. So unless there's some objections I'll
get this committed in a couple days.

+1

--
Daniel Gustafsson

On 7/7/26 23:00, Daniel Gustafsson wrote:

On 6 Jul 2026, at 13:43, Tomas Vondra <tomas@vondra.me> wrote:

Here's a rebased patch, addressing the minor bitrot, and with some very
minor adjustments.

Thanks for picking up this!

3) I've noticed the SGML docs don't have default values for the existing
parameters (cost_delay, cost_limit), so I added those. Maybe backpatch?

+1, that sounds like a good idea.

4) I've added the new 'fast' parameter to the DataChecksums module, but
mostly just for completeness. The TAP tests still use the default value,
so that it doesn't take longer.

+ spread (throttled), which may increase the time needed to enable checksums,

I think we should stick to the "data checksums" terminology for consistency.

@@ -1676,6 +1686,7 @@ DataChecksumsWorkerMain(Datum arg)

DataChecksumState->cost_delay = DataChecksumState->launch_cost_delay;
DataChecksumState->cost_limit = DataChecksumState->launch_cost_limit;
+ DataChecksumState->fast_checkpoint = DataChecksumState->launch_fast_checkpoint;
}
else
costs_updated = false;

This update will only change the fast parameter if cost_limit or cost_delay
were changed, shouldn't the fast parameter also be checked for runtime update?

Ah, good catch. I missed that. The attached v3 fixes this.

I've spent some time thinking about the default value. For a while I
thought maybe it should default to 'false' (in which case we use spread
checkpoints, which is the less disruptive option).

But that's actually futile, because the cost_delay still defaults to 0,
which means the "data page rewrite" phase won't be paced. It seems
surprising the first phase would not be throttled by default, while the
checkpoints are ...

So I ended up setting the 'fast=true' default.

I think fast=true is the right default.

Ack

This is not a very complicated patch, and it's been part of the old
patch series for quite a while. So unless there's some objections I'll
get this committed in a couple days.

+1

Thanks!

--
Tomas Vondra

On 8 Jul 2026, at 00:36, Tomas Vondra <tomas@vondra.me> wrote:
On 7/7/26 23:00, Daniel Gustafsson wrote:

This update will only change the fast parameter if cost_limit or cost_delay
were changed, shouldn't the fast parameter also be checked for runtime update?

Ah, good catch. I missed that. The attached v3 fixes this.

LGTM

--
Daniel Gustafsson

Hi Tomas,

Thanks for v3. One small documentation inconsistency in the disable
function before this goes in.

In func-admin.sgml the signature of pg_disable_data_checksums documents
the default as false:

pg_disable_data_checksums ( fast bool DEFAULT false )
but the actual default is true. pg_proc.dat sets

proargdefaults => '{true}'
and the prose you added right below the signature also says:

The default value is true.

On Wed, Jul 8, 2026 at 6:36 AM Tomas Vondra <tomas@vondra.me> wrote:

On 7/7/26 23:00, Daniel Gustafsson wrote:

On 6 Jul 2026, at 13:43, Tomas Vondra <tomas@vondra.me> wrote:

Here's a rebased patch, addressing the minor bitrot, and with some very
minor adjustments.

Thanks for picking up this!

3) I've noticed the SGML docs don't have default values for the existing
parameters (cost_delay, cost_limit), so I added those. Maybe backpatch?

+1, that sounds like a good idea.

4) I've added the new 'fast' parameter to the DataChecksums module, but
mostly just for completeness. The TAP tests still use the default value,
so that it doesn't take longer.

+ spread (throttled), which may increase the time needed to enable checksums,

I think we should stick to the "data checksums" terminology for consistency.

@@ -1676,6 +1686,7 @@ DataChecksumsWorkerMain(Datum arg)

DataChecksumState->cost_delay = DataChecksumState->launch_cost_delay;
DataChecksumState->cost_limit = DataChecksumState->launch_cost_limit;
+ DataChecksumState->fast_checkpoint = DataChecksumState->launch_fast_checkpoint;
}
else
costs_updated = false;

This update will only change the fast parameter if cost_limit or cost_delay
were changed, shouldn't the fast parameter also be checked for runtime update?

Ah, good catch. I missed that. The attached v3 fixes this.

I've spent some time thinking about the default value. For a while I
thought maybe it should default to 'false' (in which case we use spread
checkpoints, which is the less disruptive option).

But that's actually futile, because the cost_delay still defaults to 0,
which means the "data page rewrite" phase won't be paced. It seems
surprising the first phase would not be throttled by default, while the
checkpoints are ...

So I ended up setting the 'fast=true' default.

I think fast=true is the right default.

Ack

This is not a very complicated patch, and it's been part of the old
patch series for quite a while. So unless there's some objections I'll
get this committed in a couple days.

+1

Thanks!

--
Tomas Vondra

--
Regards,
Ewan Young

On 8 Jul 2026, at 13:53, Ewan Young <kdbase.hack@gmail.com> wrote:

In func-admin.sgml the signature of pg_disable_data_checksums documents
the default as false:

pg_disable_data_checksums ( fast bool DEFAULT false )
but the actual default is true. pg_proc.dat sets

Nice catch and +1, I missed that one.

--
Daniel Gustafsson

On 7/8/26 15:30, Daniel Gustafsson wrote:

On 8 Jul 2026, at 13:53, Ewan Young <kdbase.hack@gmail.com> wrote:

In func-admin.sgml the signature of pg_disable_data_checksums documents
the default as false:

pg_disable_data_checksums ( fast bool DEFAULT false )
but the actual default is true. pg_proc.dat sets

Nice catch and +1, I missed that one.

Yep, good catch. I went between true/false defaults a couple times, and
forgot to update all the places. The v4 fixes this, along with some
minor comment improvements.

I think it's ready, so I'm planning to go over it once more and push it
soon-ish.

regards

--
Tomas Vondra

On Sun, Jul 12, 2026 at 2:05 AM Tomas Vondra <tomas@vondra.me> wrote:

Yep, good catch. I went between true/false defaults a couple times, and
forgot to update all the places. The v4 fixes this, along with some
minor comment improvements.

Thanks for the update! I have a few minor comments.

  if (DataChecksumsInProgressOn())
- SetDataChecksumsOff();
+ SetDataChecksumsOff(DataChecksumState->fast_checkpoint);

The comment on DataChecksumsStateStruct says that the struct is
protected by DataChecksumsWorkerLock, and the update of
fast_checkpoint seems to be done while holding that lock.
However, the read here doesn't seem to be protected. Shouldn't this
read also be protected by DataChecksumsWorkerLock?

-        <function>pg_enable_data_checksums</function> (
<optional><parameter>cost_delay</parameter> <type>int</type>,
<parameter>cost_limit</parameter> <type>int</type></optional> )
+        <function>pg_enable_data_checksums</function> (
<optional><parameter>cost_delay</parameter> <type>int</type>
<literal>DEFAULT</literal> <literal>0</literal>,
<parameter>cost_limit</parameter> <type>int</type>
<literal>DEFAULT</literal> <literal>100</literal>,
<parameter>fast</parameter> <type>bool</type>
<literal>DEFAULT</literal> <literal>true</literal></optional> )

Isn't it be better to use "boolean" instead of "bool"? Other function
signatures in the docs seem to use "boolean". Likewise, it might be
better to use "integer" instead of "int" for consistency, here.

<sect3 id="checksums-online-system-impact">
<title>Impact on System of Online Operations</title>

Shouldn't this section also mention the checkpoint option?

Regards,

--
Fujii Masao

Hello!

On 7/11/26 20:28, Fujii Masao wrote:

On Sun, Jul 12, 2026 at 2:05 AM Tomas Vondra <tomas@vondra.me> wrote:

Yep, good catch. I went between true/false defaults a couple times, and
forgot to update all the places. The v4 fixes this, along with some
minor comment improvements.

Thanks for the update! I have a few minor comments.

if (DataChecksumsInProgressOn())
- SetDataChecksumsOff();
+ SetDataChecksumsOff(DataChecksumState->fast_checkpoint);

The comment on DataChecksumsStateStruct says that the struct is
protected by DataChecksumsWorkerLock, and the update of
fast_checkpoint seems to be done while holding that lock.
However, the read here doesn't seem to be protected. Shouldn't this
read also be protected by DataChecksumsWorkerLock?

Good question. Are you imagining some particular failure scenario, or
are you asking only because of the comment claiming the whole structure
is protected by DataChecksumsWorkerLock?

I think it's OK to access the field without a lock, because AFAICS the
field should not be updated by anyone while the worker is running. The
launcher sets it before starting any workers (and then at the end, if a
new operation was requested). So it's the "effective" fast_checkpoint
value, as seen by the worker. Only the worker itself updates it based on
the launch_fast_checkpoint change. Which is checked while holding the
lock, so that it won't miss any launch_fast_checkpoint updates.

We could acquire the lock, just to be sure. I think we'd need to do it
e.g. like this

LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
fast_checkpoint = DataChecksumState->fast_checkpoint;
LWLockRelease(DataChecksumsWorkerLock);

SetDataChecksumsOn(fast_checkpoint);

because we don't want to (can't?) hold the lock for the duration of the
main operation (which can take a long time, and we'd be unable to change
any of the fields until the lock is released).

So I don't think the lock is needed, and acquiring it per the above
example would make a difference.

But the locking protocol is bit unclear and maybe not explained well
enough. Maybe I'm missing something.

Daniel, is it OK to access the fast_checkpoint field without a lock?

-        <function>pg_enable_data_checksums</function> (
<optional><parameter>cost_delay</parameter> <type>int</type>,
<parameter>cost_limit</parameter> <type>int</type></optional> )
+        <function>pg_enable_data_checksums</function> (
<optional><parameter>cost_delay</parameter> <type>int</type>
<literal>DEFAULT</literal> <literal>0</literal>,
<parameter>cost_limit</parameter> <type>int</type>
<literal>DEFAULT</literal> <literal>100</literal>,
<parameter>fast</parameter> <type>bool</type>
<literal>DEFAULT</literal> <literal>true</literal></optional> )

Isn't it be better to use "boolean" instead of "bool"? Other function
signatures in the docs seem to use "boolean". Likewise, it might be
better to use "integer" instead of "int" for consistency, here.

Yeah, "boolean" would definitely be better. It's muscle memory from C.

<sect3 id="checksums-online-system-impact">
<title>Impact on System of Online Operations</title>

Shouldn't this section also mention the checkpoint option?

Yes, it should. I'll update the patch.

Thanks!

--
Tomas Vondra

On 11 Jul 2026, at 22:17, Tomas Vondra <tomas@vondra.me> wrote:
On 7/11/26 20:28, Fujii Masao wrote:

if (DataChecksumsInProgressOn())
- SetDataChecksumsOff();
+ SetDataChecksumsOff(DataChecksumState->fast_checkpoint);

The comment on DataChecksumsStateStruct says that the struct is
protected by DataChecksumsWorkerLock, and the update of
fast_checkpoint seems to be done while holding that lock.
However, the read here doesn't seem to be protected. Shouldn't this
read also be protected by DataChecksumsWorkerLock?

Good question. Are you imagining some particular failure scenario, or
are you asking only because of the comment claiming the whole structure
is protected by DataChecksumsWorkerLock?

I think it's OK to access the field without a lock, because AFAICS the
field should not be updated by anyone while the worker is running. The
launcher sets it before starting any workers (and then at the end, if a
new operation was requested). So it's the "effective" fast_checkpoint
value, as seen by the worker. Only the worker itself updates it based on
the launch_fast_checkpoint change. Which is checked while holding the
lock, so that it won't miss any launch_fast_checkpoint updates.

We could acquire the lock, just to be sure. I think we'd need to do it
e.g. like this

LWLockAcquire(DataChecksumsWorkerLock, LW_EXCLUSIVE);
fast_checkpoint = DataChecksumState->fast_checkpoint;
LWLockRelease(DataChecksumsWorkerLock);

SetDataChecksumsOn(fast_checkpoint);

because we don't want to (can't?) hold the lock for the duration of the
main operation (which can take a long time, and we'd be unable to change
any of the fields until the lock is released).

So I don't think the lock is needed, and acquiring it per the above
example would make a difference.

But the locking protocol is bit unclear and maybe not explained well
enough. Maybe I'm missing something.

Daniel, is it OK to access the fast_checkpoint field without a lock?

I can't think of a scenario where reading it unlocked could result in a torn
read, so I agree that it should be Ok. That being said, I think the safe
locking protocol to adapt here is to always protect DataChecksumState with the
lock, so +1 for grabbing it while holding the lock (like you outlined above)
before calling SetDataChecksumsOff().

--
Daniel Gustafsson

Hi Tomas,

I tested v4 on master at 5f14f82280: builds clean, test_checksums passes,
and with log_checkpoints the checkpoints come out as expected --
"checkpoint starting: fast force wait" with the default and "force wait"
with fast => false (the latter genuinely spread: a mere 1193 buffers took
write=26.7s, paced to checkpoint_timeout=30s x completion_target 0.9). The
runtime update Daniel caught in v2 also works end-to-end now: starting an
enable with the default and calling pg_enable_data_checksums(..., fast =>
false) mid-run makes the final checkpoint spread.

Two things for v5, since you're updating it anyway:

1) Daniel's terminology comment on v2 seems to have slipped through -- the
new fast paragraphs still say "the time needed to enable checksums" /
"disable checksums" (func-admin.sgml), while the surrounding text
consistently says "data checksums".

2) You asked whether there is a particular scenario behind Fujii's locking
question -- there is one, in exactly the site he quoted. The sequence (line
numbers from datachecksum_state.c with v4 applied):

1. a worker is mid-run, and pg_enable/disable is called again, so
launch_fast_checkpoint gets a new value;
2. the launcher decides to exit and SIGTERMs the worker (launcher_exit(),
:1025) -- but SIGTERM is asynchronous, so the worker keeps running
until its next abort check;
3. before that check, the worker runs its cost_delay/fast_checkpoint
update block (DataChecksumsWorkerMain(), :1727) and stores
fast_checkpoint under DataChecksumsWorkerLock;
4. launcher_exit(), now past the SIGTERM, reads fast_checkpoint UNLOCKED
in SetDataChecksumsOff(DataChecksumState->fast_checkpoint) at :1036.

Steps 3 and 4 race. The other three reads are safe as you argue -- no
worker can be writing the field there: the enable-path reads run only after
the workers have exited and been waited for (ProcessDatabase() ->
WaitForBackgroundWorkerShutdown()), and the disable path spawns no
per-database worker at all. Still harmless for a bool -- either the old
value or the latest requested one, and taking the lock wouldn't make that
any more deterministic -- so this doesn't change the conclusion. But if v5
takes the lock for protocol consistency, note the same unlocked read is in
four places, not one: launcher_exit() (:1036), the two SetDataChecksums*
calls in DataChecksumsWorkerLauncherMain() (:1233, :1245), and the
ProcessAllDatabases() failure path (:1354).

Two more cosmetic bits while you're editing for v5: the commit message has
"perform perform", "ckeckpoints" and "Revided"; and the new sgml paragraphs
say "The <parameter>fast</parameter> specifies ...", which renders as "The
fast specifies" -- "The <parameter>fast</parameter> parameter specifies"
matches how the docs phrase it elsewhere.

Regards,
Rui

On 7/13/26 11:08, Rui Zhao wrote:

Hi Tomas,

I tested v4 on master at 5f14f82280: builds clean, test_checksums passes,
and with log_checkpoints the checkpoints come out as expected --
"checkpoint starting: fast force wait" with the default and "force wait"
with fast => false (the latter genuinely spread: a mere 1193 buffers took
write=26.7s, paced to checkpoint_timeout=30s x completion_target 0.9). The
runtime update Daniel caught in v2 also works end-to-end now: starting an
enable with the default and calling pg_enable_data_checksums(..., fast =>
false) mid-run makes the final checkpoint spread.

Two things for v5, since you're updating it anyway:

1) Daniel's terminology comment on v2 seems to have slipped through -- the
new fast paragraphs still say "the time needed to enable checksums" /
"disable checksums" (func-admin.sgml), while the surrounding text
consistently says "data checksums".

Yeah, let's be consistent. "data checksums" is better.

2) You asked whether there is a particular scenario behind Fujii's locking
question -- there is one, in exactly the site he quoted. The sequence (line
numbers from datachecksum_state.c with v4 applied):

1. a worker is mid-run, and pg_enable/disable is called again, so
launch_fast_checkpoint gets a new value;
2. the launcher decides to exit and SIGTERMs the worker (launcher_exit(),
:1025) -- but SIGTERM is asynchronous, so the worker keeps running
until its next abort check;
3. before that check, the worker runs its cost_delay/fast_checkpoint
update block (DataChecksumsWorkerMain(), :1727) and stores
fast_checkpoint under DataChecksumsWorkerLock;
4. launcher_exit(), now past the SIGTERM, reads fast_checkpoint UNLOCKED
in SetDataChecksumsOff(DataChecksumState->fast_checkpoint) at :1036.

Steps 3 and 4 race. The other three reads are safe as you argue -- no
worker can be writing the field there: the enable-path reads run only after
the workers have exited and been waited for (ProcessDatabase() ->
WaitForBackgroundWorkerShutdown()), and the disable path spawns no
per-database worker at all. Still harmless for a bool -- either the old
value or the latest requested one, and taking the lock wouldn't make that
any more deterministic -- so this doesn't change the conclusion. But if v5
takes the lock for protocol consistency, note the same unlocked read is in
four places, not one: launcher_exit() (:1036), the two SetDataChecksums*
calls in DataChecksumsWorkerLauncherMain() (:1233, :1245), and the
ProcessAllDatabases() failure path (:1354).

Good point. I failed to realize launcher_exit() is obviously executed by
the launcher, not the worker. However, should it really be using the
fast_checkpoint flag in launcher_exit()? Shouldn't it be using the
launch_fast_checkpoint instead?

I think that's actually more correct, because that's what the launcher
was requested to use most recently. While fast_checkpoint is what the
worker "knows" about at that point, and maybe it's stale if it didn't
run the update block in DataChecksumsWorkerMain.

Ofc, reading launch_fast_checkpoint still requires a lock, because the
shmem flag is set by some other process when "starting" the launcher.

Two more cosmetic bits while you're editing for v5: the commit message has
"perform perform", "ckeckpoints" and "Revided"; and the new sgml paragraphs
say "The <parameter>fast</parameter> specifies ...", which renders as "The
fast specifies" -- "The <parameter>fast</parameter> parameter specifies"
matches how the docs phrase it elsewhere.

Thanks. You're right there's a bunch of typos, I'll fix them.

regards

--
Tomas Vondra

Hi,

Here's a v6 addressing the comments. I've split it into two parts:

0001 - While updating the "Impact on System of Online Operations" part,
I realized it does not mention checkpoints at all. Which seems wrong,
because even on 19 it can be a source of disruption. So 0001 adds this
reference, and I intend to backpatch this to 19.

FWIW the "Impact on System of Online Operations" seems a bit strange to
me. It's as if it was about impact on "System of Online Operations",
which does not seem right. I think it should be "Impact of Online
Operations on System" instead.

The other thing I noticed is that wal.sgml does not quite follow the
terminology, it talks about "checksums" and not "data checksums" in a
bunch of places. Maybe it should be cleaned up, but I haven't done
anything about that.

0002 - This is the main part for 20. It now acquires the locks even in
cases where I think it's not strictly necessary (but explaining the
reasoning would be longer than just following the locking protocol). It
also uses the launch_fast_checkpoint in launcher_exit, as explained in
the previous message. It should also fix all the typos, etc.

regards

--
Tomas Vondra

Hi Tomas,

Thanks, the new patch addresses the earlier points -- builds clean,
test_checksums passes, disabling with fast => false still spreads. Switching
launcher_exit() to launch_fast_checkpoint reads right: it's the source of
truth, set by the user's pg_enable/disable call, while fast_checkpoint is the
effective copy the worker maintains -- so reading launch_fast_checkpoint from
the launcher gets the current intent and can't race a worker write the way
reading fast_checkpoint could.

On the locking: three of the four reads take the lock -- launcher_exit()
(:1044) and the two in DataChecksumsWorkerLauncherMain() (:1248, :1275) -- but
ProcessAllDatabases() (:1388) still reads DataChecksumState->fast_checkpoint
inline, unlocked. It's safe there (no worker is running at that point), so
:1388 is just the read that slipped past the locking protocol; and where the
lock is taken it's LW_EXCLUSIVE on plain reads, where LW_SHARED would do (the
writers already hold it exclusive), :1388 included once locked. More
substantive: now that launcher_exit() reads launch_fast_checkpoint, the three
still reading fast_checkpoint (:1248, :1275, :1388) look inconsistent -- should
they move to launch_fast_checkpoint too?

One broader observation, and it predates the patch -- from the original
online-checksums commit; this one just adds fast_checkpoint to the pattern. Each
of the four non-launch_ "effective" fields mirrors a launch_ field, yet each has
at most one real reader: operation is never read (write-only at :1187/:1303),
cost_delay/cost_limit only by the worker (:1691, plus the change-check
at :1750),
and fast_checkpoint only by the launcher for its checkpoint (the worker's :1752
compare just keeps the mirror in step). The worker's use of the cost fields is a
"what did I last apply" baseline that could as easily be process-local, and in
each case the launch_ value is right there to read directly. So the whole
effective group looks like it may not need to be shared state at all
-- derivable
from the launch_ fields plus a worker-local baseline. Nothing to settle in this
patch, and maybe there's intent behind the mirror I'm missing, but since the
patch grows the group by one it seemed worth putting on the table.

One doc nit, since 0001 is backpatched: typos in the added text -- "generate of
lot of writes" -> "a lot", "requresting spread checkpoints" -> "requesting", and
the I/O bullet's "enabling and disabling data checksums requires performing
checkpoints" -> "require".

On the "checksums" vs "data checksums" wording in wal.sgml: that's the whole
section's existing style, not something this patch touches, so I'd leave it out
of a backpatched doc fix -- a separate cleanup if anyone cares. And +1 on the
retitle to "Impact of Online Operations on System".

Regards,
Rui