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

Hi,

On 2021-08-11 16:11:34 -0400, Melanie Plageman wrote:
> On Tue, Aug 3, 2021 at 2:13 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Also, I'm unsure how writing the buffer action stats out in
> > > pgstat_write_statsfiles() will work, since I think that backends can
> > > update their buffer action stats after we would have already persisted
> > > the data from the BufferActionStatsArray -- causing us to lose those
> > > updates.
> >
> > I was thinking it'd work differently. Whenever a connection ends, it reports
> > its data up to pgstats.c (otherwise we'd loose those stats). By the time
> > shutdown happens, they all need to have already have reported their stats - so
> > we don't need to do anything to get the data to pgstats.c during shutdown
> > time.
> >
>
> When you say "whenever a connection ends", what part of the code are you
> referring to specifically?

pgstat_beshutdown_hook()

> Also, when you say "shutdown", do you mean a backend shutting down or
> all backends shutting down (including postmaster) -- like pg_ctl stop?

Admittedly our language is very imprecise around this :(. What I meant
is that backends would report their own stats up to the stats collector
when the connection ends (in pgstat_beshutdown_hook()). That means that
when the whole server (pgstat and then postmaster, potentially via
pg_ctl stop) shuts down, all the per-connection stats have already been
reported up to pgstat.

> > > diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
> > > index 55f6e3711d..96cac0a74e 100644
> > > --- a/src/backend/catalog/system_views.sql
> > > +++ b/src/backend/catalog/system_views.sql
> > > @@ -1067,9 +1067,6 @@ CREATE VIEW pg_stat_bgwriter AS
> > > pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> > > pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> > > pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > > - pg_stat_get_buf_written_backend() AS buffers_backend,
> > > - pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> > > - pg_stat_get_buf_alloc() AS buffers_alloc,
> > > pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
> >
> > Material for a separate patch, not this. But if we're going to break
> > monitoring queries anyway, I think we should consider also renaming
> > maxwritten_clean (and perhaps a few others), because nobody understands what
> > that is supposed to mean.

> Do you mean I shouldn't remove anything from the pg_stat_bgwriter view?

No - I just meant that now that we're breaking pg_stat_bgwriter queries,
we should also rename the columns to be easier to understand. But that
it should be a separate patch / commit...

> > > @@ -411,11 +421,6 @@ StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc)
> > > */
> > > *complete_passes += nextVictimBuffer / NBuffers;
> > > }
> > > -
> > > - if (num_buf_alloc)
> > > - {
> > > - *num_buf_alloc = pg_atomic_exchange_u32(&StrategyControl->numBufferAllocs, 0);
> > > - }
> > > SpinLockRelease(&StrategyControl->buffer_strategy_lock);
> > > return result;
> > > }
> >
> > Hm. Isn't bgwriter using the *num_buf_alloc value to pace its activity? I
> > suspect this patch shouldn't get rid of numBufferAllocs at the same time as
> > overhauling the stats stuff. Perhaps we don't need both - but it's not obvious
> > that that's the case / how we can make that work.
> >
> >
>
> I initially meant to add a function to the patch like
> pg_stat_get_buffer_actions() but which took a BufferActionType and
> BackendType as parameters and returned a single value which is the
> number of buffer action types of that type for that type of backend.
>
> let's say I defined it like this:
> uint64
> pg_stat_get_backend_buffer_actions_stats(BackendType backend_type,
> BufferActionType ba_type)
>
> Then, I intended to use that in StrategySyncStart() to set num_buf_alloc
> by subtracting the value of StrategyControl->numBufferAllocs from the
> value returned by pg_stat_get_backend_buffer_actions_stats(B_BG_WRITER,
> BA_Alloc), val, then adding that value, val, to
> StrategyControl->numBufferAllocs.

I don't think you could restrict this to B_BG_WRITER? The whole point of
this logic is that bgwriter uses the stats for *all* backends to get the
"usage rate" for buffers, which it then uses to control how many buffers
to clean.

> I think that would have the same behavior as current, though I'm not
> sure if the performance would end up being better or worse. It wouldn't
> be atomically incrementing StrategyControl->numBufferAllocs, but it
> would do a few additional atomic operations in StrategySyncStart() than
> before. Also, we would do all the work done by
> pg_stat_get_buffer_actions() in StrategySyncStart().

I think it'd be better to separate changing the bgwriter pacing logic
(and thus numBufferAllocs) from changing the stats reporting.

> But that is called comparatively infrequently, right?

Depending on the workload not that rarely. I'm afraid this might be a
bit too expensive. It's possible we can work around that however.

Greetings,

Andres Freund