At Mon, 8 Aug 2022 11:53:19 -0400, Greg Stark <stark@mit.edu> wrote in
> I'm trying to wrap my head around the shared memory stats collector
> infrastructure from
> <20220406030008.2qxipjxo776dwnqs@alap3.anarazel.de> committed in
> 5891c7a8ed8f2d3d577e7eea34dacff12d7b6bbd.
>
> I have one question about locking -- afaics there's nothing protecting
> reading the shared memory stats. There is an lwlock protecting
> concurrent updates of the shared memory stats, but that lock isn't
> taken when we read the stats. Are we ok relying on atomic 64-bit reads
> or is there something else going on that I'm missing?
>
> In particular I'm looking at pgstat.c:847 in pgstat_fetch_entry()
> which does this:
>
> memcpy(stats_data,
> pgstat_get_entry_data(kind, entry_ref->shared_stats),
> kind_info->shared_data_len);
>
> stats_data is the returned copy of the stats entry with all the
> statistics in it. But it's copied from the shared memory location
> directly using memcpy and there's no locking or change counter or
> anything protecting this memcpy that I can see.
We take LW_SHARED while creating a snapshot of fixed-numbered
stats. On the other hand we don't for variable-numbered stats. I
agree to you, that we need that also for variable-numbered stats.
If I'm not missing something, it's strange that pgstat_lock_entry()
only takes LW_EXCLUSIVE. The atached changes the interface of
pgstat_lock_entry() but there's only one user since another read of
shared stats entry is not using reference. Thus the interface change
might be too much. If I just add bare LWLockAcquire/Release() to
pgstat_fetch_entry,the amount of the patch will be reduced.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
