On Tue, Nov 26, 2024 at 12:27:33PM -0500, Andres Freund wrote: > On 2024-11-22 20:44:34 -0600, Nathan Bossart wrote: >> D'oh, I missed that ProcNumber could be used as an index for the >> BackendStatusArray. Is the attached more like what you are imagining? > > Yes. > > I'd probably add two function header comments: > > 1) explicit caution that this is fetching information not from the snapshot > but "live" data > 2) the return value might be out of date, that the procnumber needs to be > valid and that the caller is responsible for permission checking > > I'd also add a comment do the code saying that it's fine to bypass the > changecount mechanism, because we're reading a single 4 byte integer.
I've attempted to add all these details in v3. -- nathan
>From 42c35664c8a842c4d6df9cd68a58dc6cf1e35454 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Tue, 26 Nov 2024 15:04:41 -0600 Subject: [PATCH v3 1/1] Look up backend type in pg_signal_backend() more cheaply. Commit ccd38024bc, which introduced the pg_signal_autovacuum_worker role, added a call to pgstat_get_beentry_by_proc_number() for the purpose of determining whether the targeted backend process is an autovacuum worker. This function calls pgstat_read_current_status(), which can be fairly expensive and may return cached, out-of-date information. Since we just need to look up the target backend's BackendType, and we already know its ProcNumber, we can instead inspect the BackendStatusArray directly, which is much less expensive and probably more up-to-date. There are some caveats with this approach (which are documented in the code), but it's still substantially better than before. Reported-by: Andres Freund Reviewed-by: Andres Freund Discussion: https://postgr.es/m/ujenaa2uabzfkwxwmfifawzdozh3ljr7geozlhftsuosgm7n7q%40g3utqqyyosb6 --- src/backend/storage/ipc/signalfuncs.c | 4 ++-- src/backend/utils/activity/backend_status.c | 25 +++++++++++++++++++++ src/include/utils/backend_status.h | 1 + 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index aa729a36e3..d835327600 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -88,9 +88,9 @@ 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); + BackendType backendType = pgstat_get_backend_type_by_proc_number(procNumber); - if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER) + if (backendType == 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..777f7410a7 100644 --- a/src/backend/utils/activity/backend_status.c +++ b/src/backend/utils/activity/backend_status.c @@ -1036,6 +1036,31 @@ pgstat_get_my_query_id(void) return MyBEEntry->st_query_id; } +/* ---------- + * pgstat_get_backend_type_by_proc_number() - + * + * Return the type of the backend with the specified ProcNumber. This looks + * directly at the BackendStatusArray, so the return value may be out of date. + * The only current use of this function is in pg_signal_backend(), which is + * inherently racy, so we don't worry too much about this. + * + * It is the caller's responsibility to use this wisely; at minimum, callers + * should ensure that procNumber is valid and perform the required permissions + * checking. + * ---------- + */ +BackendType +pgstat_get_backend_type_by_proc_number(ProcNumber procNumber) +{ + volatile PgBackendStatus *status = &BackendStatusArray[procNumber]; + + /* + * We bypass the changecount mechanism since fetching and storing an int + * is almost certainly atomic. + */ + return status->st_backendType; +} + /* ---------- * cmp_lbestatus * diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h index 97874300c3..4e8b39a66d 100644 --- a/src/include/utils/backend_status.h +++ b/src/include/utils/backend_status.h @@ -323,6 +323,7 @@ extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser); extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen); extern uint64 pgstat_get_my_query_id(void); +extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber); /* ---------- -- 2.39.5 (Apple Git-154)
