Re: per backend WAL statistics

Hi,

On Tue, Feb 25, 2025 at 03:50:38PM +0900, Michael Paquier wrote:
> On Mon, Feb 24, 2025 at 09:07:39AM +0000, Bertrand Drouvot wrote:
> > Now that 2421e9a51d2 is in, let's resume working in this thread. PFA a rebase to
> > make the CF bot happy. Nothing has changed since V7, V8 only removes "v7-0001" (
> > as part of 2421e9a51d2), so that v8-000N is nothing but v7-000(N+1).
> 
> v7-0001 looks sensible, so does v7-0002 with the introduction of
> PgStat_WalCounters to tackle the fact that backend statistics need
> only one reset_timestamp shared across IO and WAL stats.

Thanks for looking at it! (I guess you meant to say v8-0001 and v8-0002).

> --- a/src/backend/utils/activity/pgstat_wal.c
> +++ b/src/backend/utils/activity/pgstat_wal.c
> @@ -53,6 +53,8 @@ pgstat_report_wal(bool force)
>      /* flush wal stats */
>      pgstat_flush_wal(nowait);
>  
> +    pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_WAL);
> +
>      /* flush IO stats */
>      pgstat_flush_io(nowait);
> 
> Fine to stick that into pgstat_report_wal(), which is used anywhere
> else.  pgstat_flush_wal() could be static in pgstat_wal.c? 

hmm right. Not linked to this patch though, so done in a dedicated patch
in passing (v9-0001).

> +Datum
> +pg_stat_get_backend_wal(PG_FUNCTION_ARGS)
> +{
> [...]
> +    pid = PG_GETARG_INT32(0);
> +    proc = BackendPidGetProc(pid);
> +
> +    /*
> +     * This could be an auxiliary process but these do not report backend
> +     * statistics due to pgstat_tracks_backend_bktype(), so there is no need
> +     * for an extra call to AuxiliaryPidGetProc().
> +     */
> +    if (!proc)
> +        PG_RETURN_NULL();
> +
> +    procNumber = GetNumberFromPGProc(proc);
> +
> +    beentry = pgstat_get_beentry_by_proc_number(procNumber);
> +    if (!beentry)
> +        PG_RETURN_NULL();
> +
> +    backend_stats = pgstat_fetch_stat_backend(procNumber);
> +    if (!backend_stats)
> +        PG_RETURN_NULL();
> +
> +    /* if PID does not match, leave */
> +    if (beentry->st_procpid != pid)
> +        PG_RETURN_NULL();
> +
> +    /* backend may be gone, so recheck in case */
> +    if (beentry->st_backendType == B_INVALID)
> +        PG_RETURN_NULL();
> 
> This is a block of code copy-pasted from pg_stat_get_backend_io().
> This is complex, so perhaps it would be better to refactor that in a
> single routine that returns PgStat_Backend?  Then reuse the refactored
> code in both pg_stat_get_backend_io() and the new
> pg_stat_get_backend_wal().

That makes fully sense. Done in 0004 attached. Somehow related to that, I've
a patch in progress to address some of Rahila's comments ([1]) (the one related
to the AuxiliaryPidGetProc() call is relevant specially since a051e71e28a where
pgstat_tracks_backend_bktype() has been modified for B_WAL_RECEIVER, B_WAL_SUMMARIZER
and B_WAL_WRITER). I'll wait for 0004 to go in before sharing the patch.

> +-- Test pg_stat_get_backend_wal (and make a temp table so our temp schema exists)
> +SELECT wal_bytes AS backend_wal_bytes_before from pg_stat_get_backend_wal(pg_backend_pid()) \gset
> +
>  CREATE TEMP TABLE test_stats_temp AS SELECT 17;
>  DROP TABLE test_stats_temp;
> [...]
> +SELECT pg_stat_force_next_flush();
> +SELECT wal_bytes > :backend_wal_bytes_before FROM pg_stat_get_backend_wal(pg_backend_pid());
> 
> That should be stable, as we're guaranteed to have records here.

Yup.

[1]: https://www.postgresql.org/message-id/CAH2L28v9BwN8_y0k6FQ591%3D0g2Hj_esHLGj3bP38c9nmVykoiA%40mail.gmail.com

Regards,

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