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
