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

On Fri, Aug 13, 2021 at 3:08 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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:
> > > > 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...
>

I separated the removal of some redundant stats from pg_stat_bgwriter
into a different commit but haven't removed or clarified any additional
columns in pg_stat_bgwriter.

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

I've restored StrategyControl->numBuffersAlloc.

Attached is v6 of the patchset.

I have made several small updates to the patch, including user docs
updates, comment clarifications, various changes related to how
structures are initialized, code simplications, small details like
alphabetizing of #includes, etc.

Below are details on the remaining TODOs and open questions for this
patch and why I haven't done them yet:

1) performance testing (initial tests done, but need to do some further
investigation before sharing)

2) stats_reset
Because pg_stat_buffer_actions fields were added to the globalStats
structure, they get reset when the target RESET_BGWRITER is reset.
Depending on whether or not these commits remove columns from the
pg_stat_bgwriter view, I would approach adding stats_reset to
pg_stat_buffer_actions differently. If removing all of pg_stat_bgwriter,
I would just rename the target to apply to pg_stat_buffer_actions. If
not removing all of pg_stat_bgwriter, I would add a new target for
pg_stat_buffer_actions to reset those stats and then either remove them
from globalStats or MemSet() only the relevant parts of the struct in
pgstat_recv_resetsharedcounter().
I haven't done this yet because I want to get input on what should
happen to pg_stat_bgwriter first (all of it goes, all of it stays, some
goes, etc).

3) what to count
Currently, the patch counts allocs, extends, fsyncs and writes of shared
buffers and writes done when using a buffer access strategy. So, it is a
mix of mostly shared buffers and a few non-shared buffers. I am
wondering if it makes sense to also count extends with smgrextend()
other than those using shared buffers--for example when building an
index or when extending the free space map or visibility map. For
fsyncs, the patch does not count checkpointer fsyncs or fsyncs done from
XLogWrite().
On a related note, depending on what the view counts, the name
buffer_actions may or may not be too general.

I also feel like the BackendType B_BACKEND is a bit confusing when we
are tracking buffer actions for different backend types -- this name
makes it seem like other types of backends are not backends.

I'm not sure what the view should track and can see arguments for
excluding certain extends or separating them into another stat. I
haven't made the changes because I am looking for other peoples'
opinions.

4) Adding some sort of protection against regressions when code is added
that adds additional buffer actions but doesn't count them -- more
likely if we are counting all users of smgrextend() but not doing the
counter incrementing there.

I'm not sure how I would even do this, so, that's why I haven't done it.

5) It seems like the code to create a tuplestore used by various stats
functions like pg_stat_get_progress_info(), pg_stat_get_activity, and
pg_stat_get_slru could be refactored into a helper function since it is
quite redundant (maybe returning a ReturnSetInfo).

I haven't done this because I wasn't sure if it was a good idea, and, if
it is, if I should do it in a separate commit.

6) Cleaning up of commit message, running pgindent, and, eventually,
catalog bump (waiting until the patch is done to do this).

7) Additional testing to ensure all codepaths added are hit (one-off
testing, not added to regression test suite). I am waiting to do this
until all of the types of buffer actions that will be done are
finalized.

- Melanie