Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Hello.

At Sun, 26 Jan 2020 12:22:03 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in
> Hi,

I feel the same on the specific issues brought in upthread.

> On 2020-01-26 16:20:03 +0100, Magnus Hagander wrote:
> > On Sun, Jan 26, 2020 at 1:44 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2020-01-25 15:43:41 +0100, Magnus Hagander wrote:
> > > > On Fri, Jan 24, 2020 at 8:52 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > > Lastly, I don't understand what the point of sending fixed size stats,
> > > > > like the stuff underlying pg_stat_bgwriter, through pgstats IPC. While
> > > > > I don't like it's architecture, we obviously need something like pgstat
> > > > > to handle variable amounts of stats (database, table level etc
> > > > > stats). But that doesn't at all apply to these types of global stats.
> > > >
> > > > That part has annoyed me as well a few times. +1 for just moving that
> > > > into a global shared memory. Given that we don't really care about
> > > > things being in sync between those different counters *or* if we loose
> > > > a bit of data (which the stats collector is designed to do), we could
> > > > even do that without a lock?
> > >
> > > I don't think we'd quite want to do it without any (single counter)
> > > synchronization - high concurrency setups would be pretty likely to
> > > loose values that way. I suspect the best would be to have a struct in
> > > shared memory that contains the potential counters for each potential
> > > process. And then sum them up when actually wanting the concrete
> > > value. That way we avoid unnecessary contention, in contrast to having a
> > > single shared memory value for each(which would just pingpong between
> > > different sockets and store buffers). There's a few details like how
> > > exactly to implement resetting the counters, but ...
> >
> > Right. Each process gets to do their own write, but still in shared
> > memory. But do you need to lock them when reading them (for the
> > summary)? That's the part where I figured you could just read and
> > summarize them, and accept the possible loss.
>
> Oh, yea, I'd not lock for that. On nearly all machines aligned 64bit
> integers can be read / written without a danger of torn values, and I
> don't think we need perfect cross counter accuracy. To deal with the few
> platforms without 64bit "single copy atomicity", we can just use
> pg_atomic_read/write_u64. These days (e8fdbd58fe) they automatically
> fall back to using locked operations for those platforms. So I don't
> think there's actually a danger of loss.
>
> Obviously we could also use atomic ops to increment the value, but I'd
> rather not add all those atomic operations, even if it's on uncontended
> cachelines. It'd allow us to reset the backend values more easily by
> just swapping in a 0, which we can't do if the backend increments
> non-atomically. But I think we could instead just have one global "bias"
> value to implement resets (by subtracting that from the summarized
> value, and storing the current sum when resetting). Or use the new
> global barrier to trigger a reset. Or something similar.

Fixed or global stats are suitable for the startar of shared-memory
stats collector. In the case of buffers_*_write, the global stats
entry for each process needs just 8 bytes plus matbe extra 8 bytes for
the bias value. I'm not sure how many counters like this there are,
but is such size of footprint acceptatble? (Each backend already uses
the same amount of local memory for pgstat use, though.)

Anyway I will do something like that as a trial, maybe by adding a
member in PgBackendStatus and one global-shared for the bial value.

int64 st_progress_param[PGSTAT_NUM_PROGRESS_PARAM];
+ PgBackendStatsCounters counters;
} PgBackendStatus;

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center