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

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