> 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
