Hello!
On 7/11/26 20:28, Fujii Masao wrote:
> On Sun, Jul 12, 2026 at 2:05 AM Tomas Vondra <[email protected]> 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
