> On 11 Jul 2026, at 22:17, Tomas Vondra <[email protected]> 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
