On Fri, Nov 22, 2024 at 12:13:49PM -0500, Andres Freund wrote:
> I justed ended up looking at this code for some boring reason. One thing that
> has me worried a bit is that pg_signal_backend() now does
> pgstat_get_beentry_by_proc_number(), triggering a pgstat_read_current_status()
> further down.
>
> pgstat_read_current_status() can be fairly expensive, both in CPU and in
> memory. It copies each proc's activity strings, which each can be 1kB by
> default!
Thanks for bringing this to my attention.
> I think it'd be better to introduce something that fetches a live
> BackendType. We have such functionality already, see
> pgstat_get_backend_current_activity().
Here is a draft-grade patch for this one. It seems pretty
straightforward...
--
nathan
>From a5a3eb803b255acf4757217b9715e76f0a3c192c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Fri, 22 Nov 2024 13:19:44 -0600
Subject: [PATCH v1 1/1] avoid calling pgstat_read_current_status() in
pg_signal_backend()
---
src/backend/storage/ipc/signalfuncs.c | 5 +--
src/backend/utils/activity/backend_status.c | 37 +++++++++++++++++++++
src/include/utils/backend_status.h | 1 +
3 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/signalfuncs.c
b/src/backend/storage/ipc/signalfuncs.c
index aa729a36e3..8e165e9729 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -87,10 +87,7 @@ pg_signal_backend(int pid, int sig)
*/
if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
{
- ProcNumber procNumber = GetNumberFromPGProc(proc);
- PgBackendStatus *procStatus =
pgstat_get_beentry_by_proc_number(procNumber);
-
- if (procStatus && procStatus->st_backendType ==
B_AUTOVAC_WORKER)
+ if (pgstat_get_backend_type(pid) == B_AUTOVAC_WORKER)
{
if (!has_privs_of_role(GetUserId(),
ROLE_PG_SIGNAL_AUTOVACUUM_WORKER))
return SIGNAL_BACKEND_NOAUTOVAC;
diff --git a/src/backend/utils/activity/backend_status.c
b/src/backend/utils/activity/backend_status.c
index bdb3a296ca..4b39e30ca5 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -941,6 +941,43 @@ pgstat_get_backend_current_activity(int pid, bool
checkUser)
return "<backend information not available>";
}
+BackendType
+pgstat_get_backend_type(int pid)
+{
+ PgBackendStatus *beentry = BackendStatusArray;
+
+ for (int i = 1; i <= MaxBackends; i++)
+ {
+ volatile PgBackendStatus *vbeentry = beentry;
+ bool found;
+
+ for (;;)
+ {
+ int before_changecount;
+ int after_changecount;
+
+ pgstat_begin_read_activity(vbeentry,
before_changecount);
+
+ found = (vbeentry->st_procpid == pid);
+
+ pgstat_end_read_activity(vbeentry, after_changecount);
+
+ if (pgstat_read_activity_complete(before_changecount,
+
after_changecount))
+ break;
+
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ if (found)
+ return beentry->st_backendType;
+
+ beentry++;
+ }
+
+ return B_INVALID;
+}
+
/* ----------
* pgstat_get_crashed_backend_activity() -
*
diff --git a/src/include/utils/backend_status.h
b/src/include/utils/backend_status.h
index 97874300c3..dfa4aa6127 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -320,6 +320,7 @@ extern void pgstat_report_tempfile(size_t filesize);
extern void pgstat_report_appname(const char *appname);
extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
extern const char *pgstat_get_backend_current_activity(int pid, bool
checkUser);
+extern BackendType pgstat_get_backend_type(int pid);
extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
int buflen);
extern uint64 pgstat_get_my_query_id(void);
--
2.39.5 (Apple Git-154)
