Re: shared-memory based stats collector - v70

Hi,

On 2022-08-09 17:24:35 +0900, Kyotaro Horiguchi wrote:
> At Mon, 8 Aug 2022 11:53:19 -0400, Greg Stark <stark(at)mit(dot)edu> wrote in
> > I'm trying to wrap my head around the shared memory stats collector
> > infrastructure from
> > <20220406030008(dot)2qxipjxo776dwnqs(at)alap3(dot)anarazel(dot)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?

Yes, that's not right. Not sure how it ended up that way. There was a lot of
refactoring and pushing down the locking into different places, I guess it got
lost somewhere on the way :(. It's unlikely to be a large problem, but we
should fix it.

> If I'm not missing something, it's strange that pgstat_lock_entry()
> only takes LW_EXCLUSIVE.

I think it makes some sense, given that there's a larger number of callers for
that in various stats-emitting code. Perhaps we could just add a separate
function with a _shared() suffix?

> 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.

Could you try the pgstat_lock_entry_shared() approach?

Greetings,

Andres Freund