Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tZlNH-00CmDY-CY for pgsql-hackers@arkaria.postgresql.org; Mon, 20 Jan 2025 06:35:04 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tZlNG-00Cu1B-1i for pgsql-hackers@arkaria.postgresql.org; Mon, 20 Jan 2025 06:35:02 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tZlNE-00Cu12-Pa for pgsql-hackers@lists.postgresql.org; Mon, 20 Jan 2025 06:35:02 +0000 Received: from fhigh-a5-smtp.messagingengine.com ([103.168.172.156]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1tZlNB-000UP3-0l for pgsql-hackers@lists.postgresql.org; Mon, 20 Jan 2025 06:35:00 +0000 Received: from phl-compute-04.internal (phl-compute-04.phl.internal [10.202.2.44]) by mailfhigh.phl.internal (Postfix) with ESMTP id 89FF411400AE; Mon, 20 Jan 2025 01:34:55 -0500 (EST) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-04.internal (MEProxy); Mon, 20 Jan 2025 01:34:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paquier.xyz; h= cc:cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1737354895; x=1737441295; bh=K99QGNtshH Y0pHr1+pbZNqggl6lkyQp+8gTHRiRoRZQ=; b=CPuIaT5H3XgJujPqSrCopGw+9w R6SJTmDI/HBmeBm/TailnpObma+qot52mR1BaMUNWVl4PAr8lj9qBnhAz3oPG4CB SKopVJTPdcxQv4K0R0O+hqH4t3SfMyNOPPYQS8HK7O2xMPRISXY4apfHIWK7m67O Qd1j1LCN9/hh/WlLM4fs86rxxV0eWldXLEhSRjEnU2nUB+bJFQS7hihbjDngsYek wb9Hbwy35YZjy41LNJgsvQyM4nOxCPu4NoBMHANfDL7ygKh9UqwZgAf5V5e9ERLh KMbqeIsrDcTyhTFaFYAg5MKpR/RvdkGYpDLYamL2865PsL+TBfr9Hc/+CAEw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1737354895; x=1737441295; bh=K99QGNtshHY0pHr1+pbZNqggl6lkyQp+8gT HRiRoRZQ=; b=cJwL2M7LiITL5/HFxIiUli0GRBPYjwKzVoekBoJr4TcK876IazM QxilhhujuJRHWaECpdMyE02kmXruqkSUgTl/WRX8+qdWVF8hXU1jjspTKH+qvOTg L1Aw4QtgGAjRLmOrXzCw653D6dvC+KjXoIwvZ0tLgaNpSa8ZAcIZUSRGCJF3GpM8 qJfijvuRr/yZ5j6Mx99pTreIzI20gIoUZuNRSzfnaPpr5fDaM2azwCj2BrJvJv8P KFmh1iB5b69YiTlvU8QNv6mriM+dFnSZ1mOrhF2WbcwSUl3ZkhUWYFtNM29apS3h 8hK7OQ0siM+pZAGjwTRi6jnLeaaGzdf/GTg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrudeikedgleehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnegfrhhlucfvnfffucdljedtmdenucfjughrpeffhffvvefukfhf gggtuggjsehgtderredttddvnecuhfhrohhmpefoihgthhgrvghlucfrrghquhhivghruc eomhhitghhrggvlhesphgrqhhuihgvrhdrgiihiieqnecuggftrfgrthhtvghrnhepteel ieefudffhffhtdetleeggeegfffhkeeuveetiefgudduvedutefggeeivdejnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhhitghhrggvlhes phgrqhhuihgvrhdrgiihiidpnhgspghrtghpthhtohepiedpmhhouggvpehsmhhtphhouh htpdhrtghpthhtohepsggvrhhtrhgrnhguughrohhuvhhothdrphhgsehgmhgrihhlrdgt ohhmpdhrtghpthhtoheprghnughrvghssegrnhgrrhgriigvlhdruggvpdhrtghpthhtoh epsgihrghvuhiikedusehgmhgrihhlrdgtohhmpdhrtghpthhtoheprghlvhhhvghrrhgv segrlhhvhhdrnhhoqdhiphdrohhrghdprhgtphhtthhopehhohhrihhkhihothgrrdhnth htsehgmhgrihhlrdgtohhmpdhrtghpthhtohepphhgshhqlhdqhhgrtghkvghrsheslhhi shhtshdrphhoshhtghhrvghsqhhlrdhorhhg X-ME-Proxy: Feedback-ID: i0fe9450f:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 20 Jan 2025 01:34:52 -0500 (EST) Date: Mon, 20 Jan 2025 15:34:41 +0900 From: Michael Paquier To: Bertrand Drouvot Cc: Andres Freund , Nazir Bilal Yavuz , Alvaro Herrera , Kyotaro Horiguchi , pgsql-hackers@lists.postgresql.org Subject: Re: per backend I/O statistics Message-ID: References: <66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vKnaVkQ6QZwIFnR2" Content-Disposition: inline In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --vKnaVkQ6QZwIFnR2 Content-Type: multipart/mixed; boundary="vQDy1Se58LvK1Nog" Content-Disposition: inline --vQDy1Se58LvK1Nog Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Jan 18, 2025 at 05:53:31PM +0900, Michael Paquier wrote: > Hmm. Such special complexities in pgstat.c are annoying. There is > a stupid thing I am wondering here. For the WAL stats, why couldn't > we place some calls of pgstat_prep_backend_pending() in strategic > places like XLogBeginInsert() to force all the allocation steps of the > pending entry to happen before we would enter the critical sections > when doing a WAL insertion? As far as I can see, there is a special > case with 2PC where XLogBeginInsert() could be called in a critical > section, but that seems to be the only one at quick glance. I've looked first at this idea over the week-end with a quick hack, and it did not finish well. And then it struck me that with a bit of redesign of the callbacks, so as the existing flush_fixed_cb and have_fixed_pending_cb are usable with variable-numbered stats, we should be able to avoid the exception your first version of the patch has introduced. Attached is a patch to achieve what I have in mind, which is more generic than your previous patch. I've built my stuff as 0002 on top of your 0001. One key choice I have made is to hide PendingBackendStats within pgstat_backend.c so as it is possible to enforce some sanity checks there. -- Michael --vQDy1Se58LvK1Nog Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0001-Rework-per-backend-pending-stats.patch" Content-Transfer-Encoding: quoted-printable =46rom 90ffa6d32a9cd0034a2fd97c31991199f2089698 Mon Sep 17 00:00:00 2001 =46rom: Bertrand Drouvot Date: Fri, 17 Jan 2025 06:44:47 +0000 Subject: [PATCH v2 1/2] Rework per backend pending stats 9aea73fc61d added per backend pending statistics but not all of them are we= ll suited to memory allocation and have to be outside of critical sections. For those (the only current one is I/O statistics but WAL statistics is und= er discussion), let's rely on a new PendingBackendStats instead. --- src/include/pgstat.h | 12 +++++++++ src/backend/utils/activity/pgstat.c | 27 ++++++++++++++++++- src/backend/utils/activity/pgstat_backend.c | 29 +++++++++++++++------ src/backend/utils/activity/pgstat_io.c | 14 +++------- 4 files changed, 62 insertions(+), 20 deletions(-) diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 2d40fe3e70..3eb7b6614f 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -826,5 +826,17 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCaus= e; /* updated directly by backends and background processes */ extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats; =20 +/* + * Variables in pgstat_backend.c + */ + +/* updated directly by backends */ + +/* + * Some pending statistics are not well suited to memory allocation and ha= ve + * to be outside of critical sections. For those, let's rely on this varia= ble + * instead. + */ +extern PGDLLIMPORT PgStat_BackendPending PendingBackendStats; =20 #endif /* PGSTAT_H */ diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activi= ty/pgstat.c index 34520535d5..27aa049100 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1369,6 +1369,7 @@ static bool pgstat_flush_pending_entries(bool nowait) { bool have_pending =3D false; + bool backend_have_pending =3D false; dlist_node *cur =3D NULL; =20 /* @@ -1416,9 +1417,33 @@ pgstat_flush_pending_entries(bool nowait) cur =3D next; } =20 + /* + * There is a special case for some pending stats that are tracked in + * PendingBackendStats. It's possible that those have not been flushed + * above, hence the extra check here. + */ + if (!pg_memory_is_all_zeros(&PendingBackendStats, + sizeof(struct PgStat_BackendPending))) + { + PgStat_EntryRef *entry_ref; + + entry_ref =3D pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid, + MyProcNumber, false, NULL); + + if (entry_ref) + { + PgStat_HashKey key =3D entry_ref->shared_entry->key; + PgStat_Kind kind =3D key.kind; + const PgStat_KindInfo *kind_info =3D pgstat_get_kind_info(kind); + + if (!kind_info->flush_pending_cb(entry_ref, nowait)) + backend_have_pending =3D true; + } + } + Assert(dlist_is_empty(&pgStatPending) =3D=3D !have_pending); =20 - return have_pending; + return (have_pending || backend_have_pending); } =20 =20 diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/util= s/activity/pgstat_backend.c index 79e4d0a305..ca5cec348b 100644 --- a/src/backend/utils/activity/pgstat_backend.c +++ b/src/backend/utils/activity/pgstat_backend.c @@ -22,8 +22,11 @@ =20 #include "postgres.h" =20 +#include "utils/memutils.h" #include "utils/pgstat_internal.h" =20 +PgStat_BackendPending PendingBackendStats =3D {0}; + /* * Returns statistics of a backend by proc number. */ @@ -46,14 +49,20 @@ static void pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref) { PgStatShared_Backend *shbackendent; - PgStat_BackendPending *pendingent; PgStat_BktypeIO *bktype_shstats; - PgStat_PendingIO *pending_io; + PgStat_PendingIO pending_io; + + /* + * This function can be called even if nothing at all has happened. In + * this case, avoid unnecessarily modifying the stats entry. + */ + if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io, + sizeof(struct PgStat_PendingIO))) + return; =20 shbackendent =3D (PgStatShared_Backend *) entry_ref->shared_stats; - pendingent =3D (PgStat_BackendPending *) entry_ref->pending; bktype_shstats =3D &shbackendent->stats.io_stats; - pending_io =3D &pendingent->pending_io; + pending_io =3D PendingBackendStats.pending_io; =20 for (int io_object =3D 0; io_object < IOOBJECT_NUM_TYPES; io_object++) { @@ -64,17 +73,21 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_re= f) instr_time time; =20 bktype_shstats->counts[io_object][io_context][io_op] +=3D - pending_io->counts[io_object][io_context][io_op]; + pending_io.counts[io_object][io_context][io_op]; bktype_shstats->bytes[io_object][io_context][io_op] +=3D - pending_io->bytes[io_object][io_context][io_op]; - - time =3D pending_io->pending_times[io_object][io_context][io_op]; + pending_io.bytes[io_object][io_context][io_op]; + time =3D pending_io.pending_times[io_object][io_context][io_op]; =20 bktype_shstats->times[io_object][io_context][io_op] +=3D INSTR_TIME_GET_MICROSEC(time); } } } + + /* + * Clear out the statistics buffer, so it can be re-used. + */ + MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO)); } =20 /* diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/act= ivity/pgstat_io.c index 027aad8b24..b8f19515ae 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -75,11 +75,8 @@ pgstat_count_io_op(IOObject io_object, IOContext io_cont= ext, IOOp io_op, =20 if (pgstat_tracks_backend_bktype(MyBackendType)) { - PgStat_BackendPending *entry_ref; - - entry_ref =3D pgstat_prep_backend_pending(MyProcNumber); - entry_ref->pending_io.counts[io_object][io_context][io_op] +=3D cnt; - entry_ref->pending_io.bytes[io_object][io_context][io_op] +=3D bytes; + PendingBackendStats.pending_io.counts[io_object][io_context][io_op] +=3D= cnt; + PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] +=3D = bytes; } =20 PendingIOStats.counts[io_object][io_context][io_op] +=3D cnt; @@ -146,13 +143,8 @@ pgstat_count_io_op_time(IOObject io_object, IOContext = io_context, IOOp io_op, io_time); =20 if (pgstat_tracks_backend_bktype(MyBackendType)) - { - PgStat_BackendPending *entry_ref; - - entry_ref =3D pgstat_prep_backend_pending(MyProcNumber); - INSTR_TIME_ADD(entry_ref->pending_io.pending_times[io_object][io_contex= t][io_op], + INSTR_TIME_ADD(PendingBackendStats.pending_io.pending_times[io_object][= io_context][io_op], io_time); - } } =20 pgstat_count_io_op(io_object, io_context, io_op, cnt, bytes); --=20 2.47.1 --vQDy1Se58LvK1Nog Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0002-More-work-to-allow-backend-statistics-in-critical.patch" Content-Transfer-Encoding: quoted-printable =46rom 99561e65c3e266157ac21588097c8222a1df1d28 Mon Sep 17 00:00:00 2001 =46rom: Michael Paquier Date: Mon, 20 Jan 2025 15:31:57 +0900 Subject: [PATCH v2 2/2] More work to allow backend statistics in critical sections Callbacks are refactored so as variable-numbered stats can use them if they don't want to use pending data, like the backend stats. --- src/include/pgstat.h | 21 ++- src/include/utils/pgstat_internal.h | 29 ++-- src/backend/utils/activity/pgstat.c | 67 +++------ src/backend/utils/activity/pgstat_backend.c | 142 ++++++++++++------- src/backend/utils/activity/pgstat_io.c | 15 +- src/backend/utils/activity/pgstat_relation.c | 4 +- 6 files changed, 140 insertions(+), 138 deletions(-) diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 3eb7b6614f..d0d4515097 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -540,6 +540,15 @@ extern PgStat_ArchiverStats *pgstat_fetch_stat_archive= r(void); * Functions in pgstat_backend.c */ =20 +/* used by pgstat_io.c for I/O stats tracked in backends */ +extern void pgstat_count_backend_io_op_time(IOObject io_object, + IOContext io_context, + IOOp io_op, + instr_time io_time); +extern void pgstat_count_backend_io_op(IOObject io_object, + IOContext io_context, + IOOp io_op, uint32 cnt, + uint64 bytes); extern PgStat_Backend *pgstat_fetch_stat_backend(ProcNumber procNumber); extern bool pgstat_tracks_backend_bktype(BackendType bktype); extern void pgstat_create_backend(ProcNumber procnum); @@ -826,17 +835,5 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCaus= e; /* updated directly by backends and background processes */ extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats; =20 -/* - * Variables in pgstat_backend.c - */ - -/* updated directly by backends */ - -/* - * Some pending statistics are not well suited to memory allocation and ha= ve - * to be outside of critical sections. For those, let's rely on this varia= ble - * instead. - */ -extern PGDLLIMPORT PgStat_BackendPending PendingBackendStats; =20 #endif /* PGSTAT_H */ diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat= _internal.h index 4bb8e5c53a..000ed5b36f 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -259,8 +259,8 @@ typedef struct PgStat_KindInfo void (*init_backend_cb) (void); =20 /* - * For variable-numbered stats: flush pending stats. Required if pending - * data is used. See flush_fixed_cb for fixed-numbered stats. + * For variable-numbered stats: flush pending stats within the dshash. + * Required if pending data interacts with the pgstats dshash. */ bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait); =20 @@ -289,17 +289,19 @@ typedef struct PgStat_KindInfo void (*init_shmem_cb) (void *stats); =20 /* - * For fixed-numbered statistics: Flush pending stats. Returns true if - * some of the stats could not be flushed, due to lock contention for - * example. Optional. + * For fixed-numbered or variable-numbered statistics: Check for pending + * stats in need of flush, when these do not use the pgstats dshash. + * Returns true if there are any stats pending for flush, triggering + * flush_cb. Optional. */ - bool (*flush_fixed_cb) (bool nowait); + bool (*have_pending_cb) (void); =20 /* - * For fixed-numbered statistics: Check for pending stats in need of - * flush. Returns true if there are any stats pending for flush. Optional. + * For fixed-numbered or variable-numbered statistics: Flush pending + * stats. Returns true if some of the stats could not be flushed, due + * to lock contention for example. Optional. */ - bool (*have_fixed_pending_cb) (void); + bool (*flush_cb) (bool nowait); =20 /* * For fixed-numbered statistics: Reset All. @@ -617,10 +619,11 @@ extern void pgstat_archiver_snapshot_cb(void); #define PGSTAT_BACKEND_FLUSH_IO (1 << 0) /* Flush I/O statistics */ #define PGSTAT_BACKEND_FLUSH_ALL (PGSTAT_BACKEND_FLUSH_IO) =20 -extern void pgstat_flush_backend(bool nowait, bits32 flags); -extern PgStat_BackendPending *pgstat_prep_backend_pending(ProcNumber procn= um); -extern bool pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowai= t); -extern void pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header,= TimestampTz ts); +extern bool pgstat_flush_backend(bool nowait, bits32 flags); +extern bool pgstat_backend_flush_cb(bool nowait); +extern bool pgstat_backend_have_pending_cb(void); +extern void pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header, + TimestampTz ts); =20 /* * Functions in pgstat_bgwriter.c diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activi= ty/pgstat.c index 27aa049100..bd1ffad6f4 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -372,7 +372,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[= PGSTAT_KIND_BUILTIN_SIZE] .shared_data_len =3D sizeof(((PgStatShared_Backend *) 0)->stats), .pending_size =3D sizeof(PgStat_BackendPending), =20 - .flush_pending_cb =3D pgstat_backend_flush_cb, + .have_pending_cb =3D pgstat_backend_have_pending_cb, + .flush_cb =3D pgstat_backend_flush_cb, .reset_timestamp_cb =3D pgstat_backend_reset_timestamp_cb, }, =20 @@ -437,8 +438,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[= PGSTAT_KIND_BUILTIN_SIZE] .shared_data_off =3D offsetof(PgStatShared_IO, stats), .shared_data_len =3D sizeof(((PgStatShared_IO *) 0)->stats), =20 - .flush_fixed_cb =3D pgstat_io_flush_cb, - .have_fixed_pending_cb =3D pgstat_io_have_pending_cb, + .flush_cb =3D pgstat_io_flush_cb, + .have_pending_cb =3D pgstat_io_have_pending_cb, .init_shmem_cb =3D pgstat_io_init_shmem_cb, .reset_all_cb =3D pgstat_io_reset_all_cb, .snapshot_cb =3D pgstat_io_snapshot_cb, @@ -455,8 +456,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[= PGSTAT_KIND_BUILTIN_SIZE] .shared_data_off =3D offsetof(PgStatShared_SLRU, stats), .shared_data_len =3D sizeof(((PgStatShared_SLRU *) 0)->stats), =20 - .flush_fixed_cb =3D pgstat_slru_flush_cb, - .have_fixed_pending_cb =3D pgstat_slru_have_pending_cb, + .flush_cb =3D pgstat_slru_flush_cb, + .have_pending_cb =3D pgstat_slru_have_pending_cb, .init_shmem_cb =3D pgstat_slru_init_shmem_cb, .reset_all_cb =3D pgstat_slru_reset_all_cb, .snapshot_cb =3D pgstat_slru_snapshot_cb, @@ -474,8 +475,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[= PGSTAT_KIND_BUILTIN_SIZE] .shared_data_len =3D sizeof(((PgStatShared_Wal *) 0)->stats), =20 .init_backend_cb =3D pgstat_wal_init_backend_cb, - .flush_fixed_cb =3D pgstat_wal_flush_cb, - .have_fixed_pending_cb =3D pgstat_wal_have_pending_cb, + .flush_cb =3D pgstat_wal_flush_cb, + .have_pending_cb =3D pgstat_wal_have_pending_cb, .init_shmem_cb =3D pgstat_wal_init_shmem_cb, .reset_all_cb =3D pgstat_wal_reset_all_cb, .snapshot_cb =3D pgstat_wal_snapshot_cb, @@ -713,22 +714,17 @@ pgstat_report_stat(bool force) { bool do_flush =3D false; =20 - /* Check for pending fixed-numbered stats */ + /* Check for pending stats */ for (PgStat_Kind kind =3D PGSTAT_KIND_MIN; kind <=3D PGSTAT_KIND_MAX; ki= nd++) { const PgStat_KindInfo *kind_info =3D pgstat_get_kind_info(kind); =20 if (!kind_info) continue; - if (!kind_info->fixed_amount) - { - Assert(kind_info->have_fixed_pending_cb =3D=3D NULL); - continue; - } - if (!kind_info->have_fixed_pending_cb) + if (!kind_info->have_pending_cb) continue; =20 - if (kind_info->have_fixed_pending_cb()) + if (kind_info->have_pending_cb()) { do_flush =3D true; break; @@ -792,22 +788,20 @@ pgstat_report_stat(bool force) /* flush of variable-numbered stats */ partial_flush |=3D pgstat_flush_pending_entries(nowait); =20 - /* flush of fixed-numbered stats */ + /* + * Flush of other stats, which could be variable-numbered or + * fixed-numbered. + */ for (PgStat_Kind kind =3D PGSTAT_KIND_MIN; kind <=3D PGSTAT_KIND_MAX; kin= d++) { const PgStat_KindInfo *kind_info =3D pgstat_get_kind_info(kind); =20 if (!kind_info) continue; - if (!kind_info->fixed_amount) - { - Assert(kind_info->flush_fixed_cb =3D=3D NULL); - continue; - } - if (!kind_info->flush_fixed_cb) + if (!kind_info->flush_cb) continue; =20 - partial_flush |=3D kind_info->flush_fixed_cb(nowait); + partial_flush |=3D kind_info->flush_cb(nowait); } =20 last_flush =3D now; @@ -1369,7 +1363,6 @@ static bool pgstat_flush_pending_entries(bool nowait) { bool have_pending =3D false; - bool backend_have_pending =3D false; dlist_node *cur =3D NULL; =20 /* @@ -1417,33 +1410,9 @@ pgstat_flush_pending_entries(bool nowait) cur =3D next; } =20 - /* - * There is a special case for some pending stats that are tracked in - * PendingBackendStats. It's possible that those have not been flushed - * above, hence the extra check here. - */ - if (!pg_memory_is_all_zeros(&PendingBackendStats, - sizeof(struct PgStat_BackendPending))) - { - PgStat_EntryRef *entry_ref; - - entry_ref =3D pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid, - MyProcNumber, false, NULL); - - if (entry_ref) - { - PgStat_HashKey key =3D entry_ref->shared_entry->key; - PgStat_Kind kind =3D key.kind; - const PgStat_KindInfo *kind_info =3D pgstat_get_kind_info(kind); - - if (!kind_info->flush_pending_cb(entry_ref, nowait)) - backend_have_pending =3D true; - } - } - Assert(dlist_is_empty(&pgStatPending) =3D=3D !have_pending); =20 - return (have_pending || backend_have_pending); + return have_pending; } =20 =20 diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/util= s/activity/pgstat_backend.c index ca5cec348b..fb7abf64a0 100644 --- a/src/backend/utils/activity/pgstat_backend.c +++ b/src/backend/utils/activity/pgstat_backend.c @@ -11,7 +11,9 @@ * This statistics kind uses a proc number as object ID for the hash table * of pgstats. Entries are created each time a process is spawned, and are * dropped when the process exits. These are not written to the pgstats f= ile - * on disk. + * on disk. Pending statistics are managed without direct interactions wi= th + * the pgstats dshash, relying on PendingBackendStats instead so as it is + * possible to report data within critical sections. * * Copyright (c) 2001-2025, PostgreSQL Global Development Group * @@ -22,10 +24,53 @@ =20 #include "postgres.h" =20 +#include "storage/bufmgr.h" #include "utils/memutils.h" #include "utils/pgstat_internal.h" =20 -PgStat_BackendPending PendingBackendStats =3D {0}; +/* + * Backend statistics counts waiting to be flushed out. We assume this va= riable + * inits to zeroes. These counters may be reported within critical sectio= ns so + * we use static memory in order to avoid memory allocation. + */ +static PgStat_BackendPending PendingBackendStats =3D {0}; +static bool have_backendstats =3D false; + +/* + * Utility routines to report I/O stats for backends, kept here to avoid + * exposing PendingBackendStats to the outside world. + */ +void +pgstat_count_backend_io_op_time(IOObject io_object, IOContext io_context, + IOOp io_op, instr_time io_time) +{ + Assert(track_io_timing); + + if (!pgstat_tracks_backend_bktype(MyBackendType)) + return; + + Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op)); + + INSTR_TIME_ADD(PendingBackendStats.pending_io.pending_times[io_object][io= _context][io_op], + io_time); + + have_backendstats =3D true; +} + +void +pgstat_count_backend_io_op(IOObject io_object, IOContext io_context, + IOOp io_op, uint32 cnt, uint64 bytes) +{ + if (!pgstat_tracks_backend_bktype(MyBackendType)) + return; + + Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op)); + + PendingBackendStats.pending_io.counts[io_object][io_context][io_op] +=3D = cnt; + PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] +=3D b= ytes; + + have_backendstats =3D true; +} =20 /* * Returns statistics of a backend by proc number. @@ -53,8 +98,9 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref) PgStat_PendingIO pending_io; =20 /* - * This function can be called even if nothing at all has happened. In - * this case, avoid unnecessarily modifying the stats entry. + * This function can be called even if nothing at all has happened for + * IO statistics. In this case, avoid unnecessarily modifying the stats + * entry. */ if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io, sizeof(struct PgStat_PendingIO))) @@ -83,25 +129,29 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_r= ef) } } } - - /* - * Clear out the statistics buffer, so it can be re-used. - */ - MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO)); } =20 /* - * Wrapper routine to flush backend statistics. + * Flush out locally pending backend statistics + * + * "flags" parameter controls which statistics to flush. Returns true + * if some statistics could not be flushed. */ -static bool -pgstat_flush_backend_entry(PgStat_EntryRef *entry_ref, bool nowait, - bits32 flags) +bool +pgstat_flush_backend(bool nowait, bits32 flags) { + PgStat_EntryRef *entry_ref; + + if (!have_backendstats) + return false; + if (!pgstat_tracks_backend_bktype(MyBackendType)) return false; =20 - if (!pgstat_lock_entry(entry_ref, nowait)) - return false; + entry_ref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_BACKEND, InvalidOid, + MyProcNumber, nowait); + if (!entry_ref) + return true; =20 /* Flush requested statistics */ if (flags & PGSTAT_BACKEND_FLUSH_IO) @@ -109,36 +159,33 @@ pgstat_flush_backend_entry(PgStat_EntryRef *entry_ref= , bool nowait, =20 pgstat_unlock_entry(entry_ref); =20 - return true; + /* + * Clear out the statistics buffer, so it can be re-used. + */ + MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending)); + + have_backendstats =3D false; + return false; +} + +/* + * Check if there are any backend stats waiting for flush. + */ +bool +pgstat_backend_have_pending_cb(void) +{ + return have_backendstats; } =20 /* * Callback to flush out locally pending backend statistics. * - * If no stats have been recorded, this function returns false. + * If some stats could not be flushed, return true. */ bool -pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) +pgstat_backend_flush_cb(bool nowait) { - return pgstat_flush_backend_entry(entry_ref, nowait, PGSTAT_BACKEND_FLUSH= _ALL); -} - -/* - * Flush out locally pending backend statistics - * - * "flags" parameter controls which statistics to flush. - */ -void -pgstat_flush_backend(bool nowait, bits32 flags) -{ - PgStat_EntryRef *entry_ref; - - if (!pgstat_tracks_backend_bktype(MyBackendType)) - return; - - entry_ref =3D pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid, - MyProcNumber, false, NULL); - (void) pgstat_flush_backend_entry(entry_ref, nowait, flags); + return pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_ALL); } =20 /* @@ -150,9 +197,8 @@ pgstat_create_backend(ProcNumber procnum) PgStat_EntryRef *entry_ref; PgStatShared_Backend *shstatent; =20 - entry_ref =3D pgstat_prep_pending_entry(PGSTAT_KIND_BACKEND, InvalidOid, - procnum, NULL); - + entry_ref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_BACKEND, InvalidOid, + MyProcNumber, false); shstatent =3D (PgStatShared_Backend *) entry_ref->shared_stats; =20 /* @@ -160,20 +206,10 @@ pgstat_create_backend(ProcNumber procnum) * e.g. if we previously used this proc number. */ memset(&shstatent->stats, 0, sizeof(shstatent->stats)); -} + pgstat_unlock_entry(entry_ref); =20 -/* - * Find or create a local PgStat_BackendPending entry for proc number. - */ -PgStat_BackendPending * -pgstat_prep_backend_pending(ProcNumber procnum) -{ - PgStat_EntryRef *entry_ref; - - entry_ref =3D pgstat_prep_pending_entry(PGSTAT_KIND_BACKEND, InvalidOid, - procnum, NULL); - - return entry_ref->pending; + MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending)); + have_backendstats =3D false; } =20 /* diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/act= ivity/pgstat_io.c index b8f19515ae..6ff5d9e96a 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -73,15 +73,12 @@ pgstat_count_io_op(IOObject io_object, IOContext io_con= text, IOOp io_op, Assert(pgstat_is_ioop_tracked_in_bytes(io_op) || bytes =3D=3D 0); Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op)); =20 - if (pgstat_tracks_backend_bktype(MyBackendType)) - { - PendingBackendStats.pending_io.counts[io_object][io_context][io_op] +=3D= cnt; - PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] +=3D = bytes; - } - PendingIOStats.counts[io_object][io_context][io_op] +=3D cnt; PendingIOStats.bytes[io_object][io_context][io_op] +=3D bytes; =20 + /* Add the per-backend counts */ + pgstat_count_backend_io_op(io_object, io_context, io_op, cnt, bytes); + have_iostats =3D true; } =20 @@ -142,9 +139,9 @@ pgstat_count_io_op_time(IOObject io_object, IOContext i= o_context, IOOp io_op, INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op= ], io_time); =20 - if (pgstat_tracks_backend_bktype(MyBackendType)) - INSTR_TIME_ADD(PendingBackendStats.pending_io.pending_times[io_object][= io_context][io_op], - io_time); + /* Add the per-backend count */ + pgstat_count_backend_io_op_time(io_object, io_context, io_op, + io_time); } =20 pgstat_count_io_op(io_object, io_context, io_op, cnt, bytes); diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/uti= ls/activity/pgstat_relation.c index 09247ba097..965a7fe2c6 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -264,7 +264,7 @@ pgstat_report_vacuum(Oid tableoid, bool shared, * VACUUM command has processed all tables and committed. */ pgstat_flush_io(false); - pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO); + (void) pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO); } =20 /* @@ -351,7 +351,7 @@ pgstat_report_analyze(Relation rel, =20 /* see pgstat_report_vacuum() */ pgstat_flush_io(false); - pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO); + (void) pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO); } =20 /* --=20 2.47.1 --vQDy1Se58LvK1Nog-- --vKnaVkQ6QZwIFnR2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeN7oEACgkQnvQgOdby QH36VQ/+MexIhLKtyF3yyKXx8axH6nun7UH5i3Qg384tWQbogl4o36eUdGRf6vU/ HeuqTeTiznTjuikSd8w0VFuB5X8FHv5+MB/Z8ge8VWwOZ9Sr19Bt3xrKN9hyJbiP FDJyB5c9TKfoJBwJVAjclCawibioy2/b3zWTEpKaQ6ZtR62fPosRuuyzCCmLtnO2 98w5bZJpagqTULs/vpJ64Hw2vq3zIH9JGbPoBh9GHqvyQIZ/Lo0QvH7b9iAMaZmP eEnpg+/NoWTTw8gVVTzAXn7OVWvkwxE0/K2Db+hlWLxVYhkWcNXM+zBkxm7oD9VG fd6Tc7f665F+xC9qfcZR8beP0iNUpUik3E3hkTXhH/xZR5xT7yAxzDGdlHvRWYxn qYGJbcumoqPXpywMMRGMRlxufvry9q8na/r8eSSGLJB0W3VFwfZnM1tuwD78qhjG vPBOGLlTKzB/gFcyUBl48ycbF/3fJT1HBUH/Adf8Q9Rkp6TOSviBKV2gayhFMfLA D8KskT4dn835rR54QtdExb3F0lRpL3g/1zHLcURL5J1orzPzkL2aeMRUqq2jcIrF Ke2ONjSyPh7Cn//3SjSy9whws5+VrGPOTuqyWflArh2VPvKRlkKqKj5m51dJZWod J8m6Egq9NTKGJnCMMCsOSV3JzZbeyAWf0JneGFWOjEAK4FCD8kU= =NUOb -----END PGP SIGNATURE----- --vKnaVkQ6QZwIFnR2--