Horton

Re: per backend WAL statistics

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

View thread View on postgr.es
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2025-01-08T11:11:59Z
Lists: pgsql-hackers

Attachments

Hi,

On Wed, Jan 08, 2025 at 03:21:26PM +0900, Michael Paquier wrote:
> On Tue, Jan 07, 2025 at 08:48:51AM +0000, Bertrand Drouvot wrote:
> > Now that commit 9aea73fc61 added backend-level statistics to pgstats (and
> > per backend IO statistics), we can more easily add per backend statistics.
> > 
> > Please find attached a patch to implement $SUBJECT.
> 
> I've looked at v1-0002 and v1-0001.

Thanks for looking at it!

> +static void
> +pgstat_flush_io_entry(PgStat_EntryRef *entry_ref, bool nowait, bool need_lock)
>  {
> -	PgStatShared_Backend *shbackendioent;
> -	PgStat_BackendPendingIO *pendingent;
> +	PgStatShared_Backend *shbackendent;
> +	PgStat_BackendPending *pendingent;
>  	PgStat_BktypeIO *bktype_shstats;
> +	PgStat_BackendPendingIO *pending_io;
>  
> -	if (!pgstat_lock_entry(entry_ref, nowait))
> -		return false;
> +	if (need_lock && !pgstat_lock_entry(entry_ref, nowait))
> +		return;
> 
> The addition of need_lock at this level leads to a result that seems a
> bit confusing, where pgstat_backend_flush_cb() passes "false" because
> it locks the entry by itself as an effect of v1-0003 with the new area
> for WAL.  Wouldn't it be cleaner to do an extra pgstat_[un]lock_entry
> dance in pgstat_backend_flush_io() instead?  Another approach I can
> think of that would be slightly cleaner to me is to pass a bits32 to a
> single routine that would control if WAL stats, I/O stats or both
> should be flushed, keeping pgstat_flush_backend() as name with an
> extra argument to decide which parts of the stats should be flushed.

Yeah, that's more elegant as it also means that the main callback will not change
(should we add even more stats in the future). Done that way in v2 attached.

> -PgStat_BackendPendingIO *
> +PgStat_BackendPending *
> 
> This rename makes sense.
> 
>  #define PG_STAT_GET_WAL_COLS	9
>  	TupleDesc	tupdesc;
>  	Datum		values[PG_STAT_GET_WAL_COLS] = {0};
>  	bool		nulls[PG_STAT_GET_WAL_COLS] = {0};
> 
> It feels unnatural to have a PG_STAT_GET_WAL_COLS while it would not
> only relate to this function anymore.

Has been renamed to PG_STAT_WAL_COLS in the attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Commits

  1. Fix use-after-free in pgstat_fetch_stat_backend_by_pid()

  2. Remove initialization from PendingBackendStats

  3. Add WAL data to backend statistics

  4. Improve check for detection of pending data in backend statistics

  5. Fix some gaps in pg_stat_io with WAL receiver and WAL summarizer

  6. Handle auxiliary processes in SQL functions of backend statistics

  7. Invent pgstat_fetch_stat_backend_by_pid()

  8. Refactor code of pg_stat_get_wal() building result tuple

  9. Adding new PgStat_WalCounters structure in pgstat.h

  10. Remove pgstat_flush_wal()

  11. Refactor some code related to backend statistics

  12. Add backend-level statistics to pgstats