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

Hi,

On 2022-07-14 18:44:48 -0400, Melanie Plageman wrote:
> Subject: [PATCH v26 1/4] Add BackendType for standalone backends
> Subject: [PATCH v26 2/4] Remove unneeded call to pgstat_report_wal()

LGTM.


> Subject: [PATCH v26 3/4] Track IO operation statistics

> @@ -978,8 +979,17 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>  
>  	bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
>  
> +	if (isLocalBuf)
> +		io_path = IOPATH_LOCAL;
> +	else if (strategy != NULL)
> +		io_path = IOPATH_STRATEGY;
> +	else
> +		io_path = IOPATH_SHARED;

Seems a bit ugly to have an if (isLocalBuf) just after an isLocalBuf ?.


> +			/*
> +			 * When a strategy is in use, reused buffers from the strategy ring will
> +			 * be counted as allocations for the purposes of IO Operation statistics
> +			 * tracking.
> +			 *
> +			 * However, even when a strategy is in use, if a new buffer must be
> +			 * allocated from shared buffers and added to the ring, this is counted
> +			 * as a IOPATH_SHARED allocation.
> +			 */

There's a bit too much duplication between the paragraphs...

> @@ -628,6 +637,9 @@ pgstat_report_stat(bool force)
>  	/* flush database / relation / function / ... stats */
>  	partial_flush |= pgstat_flush_pending_entries(nowait);
>  
> +	/* flush IO Operations stats */
> +	partial_flush |= pgstat_flush_io_ops(nowait);

Could you either add a note to the commit message that the stats file
version needs to be increased, or just iclude that in the patch.




> @@ -1427,8 +1445,10 @@ pgstat_read_statsfile(void)
>  	FILE	   *fpin;
>  	int32		format_id;
>  	bool		found;
> +	PgStat_BackendIOPathOps io_stats;
>  	const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME;
>  	PgStat_ShmemControl *shmem = pgStatLocal.shmem;
> +	PgStatShared_BackendIOPathOps *io_stats_shmem = &shmem->io_ops;
>  
>  	/* shouldn't be called from postmaster */
>  	Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
> @@ -1486,6 +1506,22 @@ pgstat_read_statsfile(void)
>  	if (!read_chunk_s(fpin, &shmem->checkpointer.stats))
>  		goto error;
>  
> +	/*
> +	 * Read IO Operations stats struct
> +	 */
> +	if (!read_chunk_s(fpin, &io_stats))
> +		goto error;
> +
> +	io_stats_shmem->stat_reset_timestamp = io_stats.stat_reset_timestamp;
> +
> +	for (int i = 0; i < BACKEND_NUM_TYPES; i++)
> +	{
> +		PgStat_IOPathOps *stats = &io_stats.stats[i];
> +		PgStatShared_IOPathOps *stats_shmem = &io_stats_shmem->stats[i];
> +
> +		memcpy(stats_shmem->data, stats->data, sizeof(stats->data));
> +	}

Why can't the data be read directly into shared memory?


>  	/*


> +void
> +pgstat_io_ops_snapshot_cb(void)
> +{
> +	PgStatShared_BackendIOPathOps *all_backend_stats_shmem = &pgStatLocal.shmem->io_ops;
> +	PgStat_BackendIOPathOps *all_backend_stats_snap = &pgStatLocal.snapshot.io_ops;
> +
> +	for (int i = 0; i < BACKEND_NUM_TYPES; i++)
> +	{
> +		PgStatShared_IOPathOps *stats_shmem = &all_backend_stats_shmem->stats[i];
> +		PgStat_IOPathOps *stats_snap = &all_backend_stats_snap->stats[i];
> +
> +		LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);

Why acquire the same lock repeatedly for each type, rather than once for
the whole?


> +		/*
> +		 * Use the lock in the first BackendType's PgStat_IOPathOps to protect the
> +		 * reset timestamp as well.
> +		 */
> +		if (i == 0)
> +			all_backend_stats_snap->stat_reset_timestamp = all_backend_stats_shmem->stat_reset_timestamp;

Which also would make this look a bit less awkward.

Starting to look pretty good...

- Andres