Re: per backend WAL statistics

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