Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) - Mailing list pgsql-hackers

Hi,

On 2021-08-02 18:25:56 -0400, Melanie Plageman wrote:
> Thanks for the feedback!
> 
> I agree it makes sense to count strategy writes separately.
> 
> I thought about this some more, and I don't know if it makes sense to
> only count "avoidable" strategy writes.
> 
> This would mean that a backend writing out a buffer from the strategy
> ring when no clean shared buffers (as well as no clean strategy buffers)
> are available would not count that write as a strategy write (even
> though it is writing out a buffer from its strategy ring). But, it
> obviously doesn't make sense to count it as a regular buffer being
> written out. So, I plan to change this code.

What do you mean with "no clean shared buffers ... are available"?



> The most substantial missing piece of the patch right now is persisting
> the data across reboots.
> 
> The two places in the code I can see to persist the buffer action stats
> data are:
> 1) using the stats collector code (like in
> pgstat_read/write_statsfiles()
> 2) using a before_shmem_exit() hook which writes the data structure to a
> file and then read from it when making the shared memory array initially

I think it's pretty clear that we should go for 1. Having two mechanisms for
persisting stats data is a bad idea.


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


> And, I don't think I can use pgstat_read_statsfiles() since the
> BufferActionStatsArray should have the data from the file as soon as the
> view containing the buffer action stats can be queried. Thus, it seems
> like I would need to read the file while initializing the array in
> CreateBufferActionStatsCounters().

Why would backends need to read that data back?


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



> @@ -1089,10 +1077,6 @@ ForwardSyncRequest(const FileTag *ftag, SyncRequestType type)
>  
>      LWLockAcquire(CheckpointerCommLock, LW_EXCLUSIVE);
>  
> -    /* Count all backend writes regardless of if they fit in the queue */
> -    if (!AmBackgroundWriterProcess())
> -        CheckpointerShmem->num_backend_writes++;
> -
>      /*
>       * If the checkpointer isn't running or the request queue is full, the
>       * backend will have to perform its own fsync request.  But before forcing
> @@ -1106,8 +1090,10 @@ ForwardSyncRequest(const FileTag *ftag, SyncRequestType type)
>           * Count the subset of writes where backends have to do their own
>           * fsync
>           */
> +        /* TODO: should we count fsyncs for all types of procs? */
>          if (!AmBackgroundWriterProcess())
> -            CheckpointerShmem->num_backend_fsync++;
> +            pgstat_increment_buffer_action(BA_Fsync);
> +

Yes, I think that'd make sense. Now that we can disambiguate the different
types of syncs between procs, I don't see a point of having a process-type
filter here. We just loose data...



>          /* don't set checksum for all-zero page */
> @@ -1229,11 +1234,60 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>                      if (XLogNeedsFlush(lsn) &&
>                          StrategyRejectBuffer(strategy, buf))
>                      {
> +                        /*
> +                         * Unset the strat write flag, as we will not be writing
> +                         * this particular buffer from our ring out and may end
> +                         * up having to find a buffer from main shared buffers,
> +                         * which, if it is dirty, we may have to write out, which
> +                         * could have been prevented by checkpointing and background
> +                         * writing
> +                         */
> +                        StrategyUnChooseBufferFromRing(strategy);
> +
>                          /* Drop lock/pin and loop around for another buffer */
>                          LWLockRelease(BufferDescriptorGetContentLock(buf));
>                          UnpinBuffer(buf, true);
>                          continue;
>                      }

Could we combine this with StrategyRejectBuffer()? It seems a bit wasteful to
have two function calls into freelist.c when the second happens exactly when
the first returns true?


> +
> +                    /*
> +                     * TODO: there is certainly a better way to write this
> +                     * logic
> +                     */
> +
> +                    /*
> +                     * The dirty buffer that will be written out was selected
> +                     * from the ring and we did not bother checking the
> +                     * freelist or doing a clock sweep to look for a clean
> +                     * buffer to use, thus, this write will be counted as a
> +                     * strategy write -- one that may be unnecessary without a
> +                     * strategy
> +                     */
> +                    if (StrategyIsBufferFromRing(strategy))
> +                    {
> +                        pgstat_increment_buffer_action(BA_Write_Strat);
> +                    }
> +
> +                        /*
> +                         * If the dirty buffer was one we grabbed from the
> +                         * freelist or through a clock sweep, it could have been
> +                         * written out by bgwriter or checkpointer, thus, we will
> +                         * count it as a regular write
> +                         */
> +                    else
> +                        pgstat_increment_buffer_action(BA_Write);

It seems this would be better solved by having an "bool *from_ring" or
GetBufferSource* parameter to StrategyGetBuffer().


> @@ -2895,6 +2948,20 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
>      /*
>       * bufToWrite is either the shared buffer or a copy, as appropriate.
>       */
> +
> +    /*
> +     * TODO: consider that if we did not need to distinguish between a buffer
> +     * flushed that was grabbed from the ring buffer and written out as part
> +     * of a strategy which was not from main Shared Buffers (and thus
> +     * preventable by bgwriter or checkpointer), then we could move all calls
> +     * to pgstat_increment_buffer_action() here except for the one for
> +     * extends, which would remain in ReadBuffer_common() before smgrextend()
> +     * (unless we decide to start counting other extends). That includes the
> +     * call to count buffers written by bgwriter and checkpointer which go
> +     * through FlushBuffer() but not BufferAlloc(). That would make it
> +     * simpler. Perhaps instead we can find somewhere else to indicate that
> +     * the buffer is from the ring of buffers to reuse.
> +     */
>      smgrwrite(reln,
>                buf->tag.forkNum,
>                buf->tag.blockNum,

Can we just add a parameter to FlushBuffer indicating what the source of the
write is?


> @@ -247,7 +257,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state)
>       * the rate of buffer consumption.  Note that buffers recycled by a
>       * strategy object are intentionally not counted here.
>       */
> -    pg_atomic_fetch_add_u32(&StrategyControl->numBufferAllocs, 1);
> +    pgstat_increment_buffer_action(BA_Alloc);
>  
>      /*
>       * First check, without acquiring the lock, whether there's buffers in the

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




> +void
> +pgstat_increment_buffer_action(BufferActionType ba_type)
> +{
> +    volatile PgBackendStatus *beentry   = MyBEEntry;
> +
> +    if (!beentry || !pgstat_track_activities)
> +        return;
> +
> +    if (ba_type == BA_Alloc)
> +        pg_atomic_add_fetch_u64(&beentry->buffer_action_stats.allocs, 1);
> +    else if (ba_type == BA_Extend)
> +        pg_atomic_add_fetch_u64(&beentry->buffer_action_stats.extends, 1);
> +    else if (ba_type == BA_Fsync)
> +        pg_atomic_add_fetch_u64(&beentry->buffer_action_stats.fsyncs, 1);
> +    else if (ba_type == BA_Write)
> +        pg_atomic_add_fetch_u64(&beentry->buffer_action_stats.writes, 1);
> +    else if (ba_type == BA_Write_Strat)
> +        pg_atomic_add_fetch_u64(&beentry->buffer_action_stats.writes_strat, 1);
> +}

I don't think we want to use atomic increments here - they're *slow*. And
there only ever can be a single writer to a backend's stats. So just doing
something like
    pg_atomic_write_u64(&var, pg_atomic_read_u64(&var) + 1)
should do the trick.


> +/*
> + * Called for a single backend at the time of death to persist its I/O stats
> + */
> +void
> +pgstat_record_dead_backend_buffer_actions(void)
> +{
> +    volatile PgBackendBufferActionStats *ba_stats;
> +    volatile    PgBackendStatus *beentry = MyBEEntry;
> +
> +    if (beentry->st_procpid != 0)
> +        return;
> +
> +    // TODO: is this correct? could there be a data race? do I need a lock?
> +    ba_stats = &BufferActionStatsArray[beentry->st_backendType];
> +    pg_atomic_add_fetch_u64(&ba_stats->allocs, pg_atomic_read_u64(&beentry->buffer_action_stats.allocs));
> +    pg_atomic_add_fetch_u64(&ba_stats->extends, pg_atomic_read_u64(&beentry->buffer_action_stats.extends));
> +    pg_atomic_add_fetch_u64(&ba_stats->fsyncs, pg_atomic_read_u64(&beentry->buffer_action_stats.fsyncs));
> +    pg_atomic_add_fetch_u64(&ba_stats->writes, pg_atomic_read_u64(&beentry->buffer_action_stats.writes));
> +    pg_atomic_add_fetch_u64(&ba_stats->writes_strat,
pg_atomic_read_u64(&beentry->buffer_action_stats.writes_strat));
> +}

I don't see a race, FWIW.

This is where I propose that we instead report the values up to the stats
collector, instead of having a separate array that we need to persist


> +/*
> + * Fill the provided values array with the accumulated counts of buffer actions
> + * taken by all backends of type backend_type (input parameter), both alive and
> + * dead. This is currently only used by pg_stat_get_buffer_actions() to create
> + * the rows in the pg_stat_buffer_actions system view.
> + */
> +void
> +pgstat_recount_all_buffer_actions(BackendType backend_type, Datum *values)
> +{
> +    int            i;
> +    volatile PgBackendStatus *beentry;
> +
> +    /*
> +     * Add stats from all exited backends
> +     */
> +    values[BA_Alloc] = pg_atomic_read_u64(&BufferActionStatsArray[backend_type].allocs);
> +    values[BA_Extend] = pg_atomic_read_u64(&BufferActionStatsArray[backend_type].extends);
> +    values[BA_Fsync] = pg_atomic_read_u64(&BufferActionStatsArray[backend_type].fsyncs);
> +    values[BA_Write] = pg_atomic_read_u64(&BufferActionStatsArray[backend_type].writes);
> +    values[BA_Write_Strat] = pg_atomic_read_u64(&BufferActionStatsArray[backend_type].writes_strat);
> +
> +    /*
> +     * Loop through all live backends and count their buffer actions
> +     */
> +    // TODO: see note in pg_stat_get_buffer_actions() about inefficiency of this method
> +
> +    beentry = BackendStatusArray;
> +    for (i = 1; i <= MaxBackends; i++)
> +    {
> +        /* Don't count dead backends. They should already be counted */
> +        if (beentry->st_procpid == 0)
> +            continue;
> +        if (beentry->st_backendType != backend_type)
> +            continue;
> +
> +        values[BA_Alloc] += pg_atomic_read_u64(&beentry->buffer_action_stats.allocs);
> +        values[BA_Extend] += pg_atomic_read_u64(&beentry->buffer_action_stats.extends);
> +        values[BA_Fsync] += pg_atomic_read_u64(&beentry->buffer_action_stats.fsyncs);
> +        values[BA_Write] += pg_atomic_read_u64(&beentry->buffer_action_stats.writes);
> +        values[BA_Write_Strat] += pg_atomic_read_u64(&beentry->buffer_action_stats.writes_strat);
> +
> +        beentry++;
> +    }
> +}

It seems to make a bit more sense to have this sum up the stats for all
backend types at once.

> +        /*
> +         * Currently, the only supported backend types for stats are the following.
> +         * If this were to change, pg_proc.dat would need to be changed as well
> +         * to reflect the new expected number of rows.
> +         */
> +        Datum values[BUFFER_ACTION_NUM_TYPES];
> +        bool nulls[BUFFER_ACTION_NUM_TYPES];

Ah ;)

Greetings,

Andres Freund