From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Imseih (AWS), Sami <simseih@amazon.com>
Cc: Ian Lawrence Barwick <barwick@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: robertmhaas@gmail.com <robertmhaas@gmail.com>
Subject: Re: pg_stat_get_backend_subxact() and backend IDs?
Date: Tue, 29 Aug 2023 19:01:51 -0700
Message-ID: <20230830020151.GA2488217@nathanxps13> (raw)
In-Reply-To: <ZO59s6in+aACdCGb@paquier.xyz>
References: <20230824023206.GA1128857@nathanxps13.lan>
<20230824025140.GB1128857@nathanxps13.lan>
<20230824161913.GA1394441@nathanxps13.lan>
<CAB8KJ=iWYvB1odjFRZybsPeUV8HUAiVVjY9RMHSdvWy1t77cLg@mail.gmail.com>
<20230825153251.GA1593219@nathanxps13>
<20230825192949.GA1917227@nathanxps13>
<CF0C64D6-BB5C-4F84-98BB-8852A1C01A40@amazon.com>
<ZOv+MHonWVgW/bpf@paquier.xyz>
<20230829164655.GA2138073@nathanxps13>
<ZO59s6in+aACdCGb@paquier.xyz>
On Wed, Aug 30, 2023 at 08:22:27AM +0900, Michael Paquier wrote:
> On Tue, Aug 29, 2023 at 09:46:55AM -0700, Nathan Bossart wrote:
>> This was my first reaction [0]. I was concerned about renaming the
>> exported functions so close to release, so I was suggesting that we hold
>> off on that part until v17. If there isn't a concern with renaming these
>> functions in v16, I can proceed with something more like v2.
>
> Thanks for the pointer. This version is much better IMO, because it
> removes entirely the source of the confusion between the difference in
> backend ID and index ID treatments when fetching the local entries in
> the array. So I'm okay to rename these functions now, before .0 is
> released to get things in a better shape while addressing the issue
> reported.
Okay.
> +extern LocalPgBackendStatus *pgstat_get_local_beentry_by_index(int beid);
>
> Still I would to a bit more of s/beid/id/ for cases where the code
> refers to an index ID, and not a backend ID, especially for the
> internal routines.
Makes sense. I did this in v4.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.comAttachments:download all (2 files, .tar.gz)v4-0001-rename-some-pgstat-functions.patch (text/x-diff, 10.1K) download
From 3e360c1b970526079528159fe1c49be03d0b13b5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 24 Aug 2023 07:42:54 -0700
Subject: [PATCH v4 1/2] rename some pgstat functions
---
src/backend/utils/activity/backend_status.c | 28 ++++++++--------
src/backend/utils/adt/pgstatfuncs.c | 36 ++++++++++-----------
src/include/utils/backend_status.h | 4 +--
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.cindex 38f91a495b..a4860b10fc 100644--- a/src/backend/utils/activity/backend_status.c+++ b/src/backend/utils/activity/backend_status.c@@ -849,8 +849,8 @@ pgstat_read_current_status(void)
/*
* The BackendStatusArray index is exactly the BackendId of the
* source backend. Note that this means localBackendStatusTable
- * is in order by backend_id. pgstat_fetch_stat_beentry() depends- * on that.+ * is in order by backend_id. pgstat_get_beentry_by_backend_id()+ * depends on that.
*/
localentry->backend_id = i;
BackendIdGetTransactionIds(i,
@@ -1073,21 +1073,21 @@ cmp_lbestatus(const void *a, const void *b)
}
/* ----------
- * pgstat_fetch_stat_beentry() -+ * pgstat_get_beentry_by_backend_id() -
*
* Support function for the SQL-callable pgstat* functions. Returns
* our local copy of the current-activity entry for one backend,
* or NULL if the given beid doesn't identify any known session.
*
* The beid argument is the BackendId of the desired session
- * (note that this is unlike pgstat_fetch_stat_local_beentry()).+ * (note that this is unlike pgstat_get_local_beentry_by_index()).
*
* NB: caller is responsible for a check if the user is permitted to see
* this info (especially the querystring).
* ----------
*/
PgBackendStatus *
-pgstat_fetch_stat_beentry(BackendId beid)+pgstat_get_beentry_by_backend_id(BackendId beid)
{
LocalPgBackendStatus key;
LocalPgBackendStatus *ret;
@@ -1111,13 +1111,13 @@ pgstat_fetch_stat_beentry(BackendId beid)
/* ----------
- * pgstat_fetch_stat_local_beentry() -+ * pgstat_get_local_beentry_by_index() -
*
- * Like pgstat_fetch_stat_beentry() but with locally computed additions (like- * xid and xmin values of the backend)+ * Like pgstat_get_beentry_by_backend_id() but with locally computed additions+ * (like xid and xmin values of the backend)
*
- * The beid argument is a 1-based index in the localBackendStatusTable- * (note that this is unlike pgstat_fetch_stat_beentry()).+ * The idx argument is a 1-based index in the localBackendStatusTable+ * (note that this is unlike pgstat_get_beentry_by_backend_id()).
* Returns NULL if the argument is out of range (no current caller does that).
*
* NB: caller is responsible for a check if the user is permitted to see
@@ -1125,14 +1125,14 @@ pgstat_fetch_stat_beentry(BackendId beid)
* ----------
*/
LocalPgBackendStatus *
-pgstat_fetch_stat_local_beentry(int beid)+pgstat_get_local_beentry_by_index(int idx)
{
pgstat_read_current_status();
- if (beid < 1 || beid > localNumBackends)+ if (idx < 1 || idx > localNumBackends)
return NULL;
- return &localBackendStatusTable[beid - 1];+ return &localBackendStatusTable[idx - 1];
}
@@ -1141,7 +1141,7 @@ pgstat_fetch_stat_local_beentry(int beid)
*
* Support function for the SQL-callable pgstat* functions. Returns
* the number of sessions known in the localBackendStatusTable, i.e.
- * the maximum 1-based index to pass to pgstat_fetch_stat_local_beentry().+ * the maximum 1-based index to pass to pgstat_get_local_beentry_by_index().
* ----------
*/
int
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.cindex 2b9742ad21..49cc887b97 100644--- a/src/backend/utils/adt/pgstatfuncs.c+++ b/src/backend/utils/adt/pgstatfuncs.c@@ -211,7 +211,7 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
if (fctx[0] <= pgstat_fetch_stat_numbackends())
{
/* do when there is more left to send */
- LocalPgBackendStatus *local_beentry = pgstat_fetch_stat_local_beentry(fctx[0]);+ LocalPgBackendStatus *local_beentry = pgstat_get_local_beentry_by_index(fctx[0]);
SRF_RETURN_NEXT(funcctx, Int32GetDatum(local_beentry->backend_id));
}
@@ -264,7 +264,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
bool nulls[PG_STAT_GET_PROGRESS_COLS] = {0};
int i;
- local_beentry = pgstat_fetch_stat_local_beentry(curr_backend);+ local_beentry = pgstat_get_local_beentry_by_index(curr_backend);
beentry = &local_beentry->backendStatus;
/*
@@ -325,7 +325,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
const char *wait_event = NULL;
/* Get the next one in the list */
- local_beentry = pgstat_fetch_stat_local_beentry(curr_backend);+ local_beentry = pgstat_get_local_beentry_by_index(curr_backend);
beentry = &local_beentry->backendStatus;
/* If looking for specific PID, ignore all the others */
@@ -672,7 +672,7 @@ pg_stat_get_backend_pid(PG_FUNCTION_ARGS)
int32 beid = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
PG_RETURN_INT32(beentry->st_procpid);
@@ -685,7 +685,7 @@ pg_stat_get_backend_dbid(PG_FUNCTION_ARGS)
int32 beid = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
PG_RETURN_OID(beentry->st_databaseid);
@@ -698,7 +698,7 @@ pg_stat_get_backend_userid(PG_FUNCTION_ARGS)
int32 beid = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
PG_RETURN_OID(beentry->st_userid);
@@ -727,7 +727,7 @@ pg_stat_get_backend_subxact(PG_FUNCTION_ARGS)
BlessTupleDesc(tupdesc);
- if ((local_beentry = pgstat_fetch_stat_local_beentry(beid)) != NULL)+ if ((local_beentry = pgstat_get_local_beentry_by_index(beid)) != NULL)
{
/* Fill values and NULLs */
values[0] = Int32GetDatum(local_beentry->backend_subxact_count);
@@ -752,7 +752,7 @@ pg_stat_get_backend_activity(PG_FUNCTION_ARGS)
char *clipped_activity;
text *ret;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
activity = "<backend information not available>";
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
activity = "<insufficient privilege>";
@@ -776,7 +776,7 @@ pg_stat_get_backend_wait_event_type(PG_FUNCTION_ARGS)
PGPROC *proc;
const char *wait_event_type = NULL;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
wait_event_type = "<backend information not available>";
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
wait_event_type = "<insufficient privilege>";
@@ -797,7 +797,7 @@ pg_stat_get_backend_wait_event(PG_FUNCTION_ARGS)
PGPROC *proc;
const char *wait_event = NULL;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
wait_event = "<backend information not available>";
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
wait_event = "<insufficient privilege>";
@@ -818,7 +818,7 @@ pg_stat_get_backend_activity_start(PG_FUNCTION_ARGS)
TimestampTz result;
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
@@ -844,7 +844,7 @@ pg_stat_get_backend_xact_start(PG_FUNCTION_ARGS)
TimestampTz result;
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
@@ -866,7 +866,7 @@ pg_stat_get_backend_start(PG_FUNCTION_ARGS)
TimestampTz result;
PgBackendStatus *beentry;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
@@ -890,7 +890,7 @@ pg_stat_get_backend_client_addr(PG_FUNCTION_ARGS)
char remote_host[NI_MAXHOST];
int ret;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
@@ -935,7 +935,7 @@ pg_stat_get_backend_client_port(PG_FUNCTION_ARGS)
char remote_port[NI_MAXSERV];
int ret;
- if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)+ if ((beentry = pgstat_get_beentry_by_backend_id(beid)) == NULL)
PG_RETURN_NULL();
else if (!HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
@@ -978,12 +978,12 @@ pg_stat_get_db_numbackends(PG_FUNCTION_ARGS)
Oid dbid = PG_GETARG_OID(0);
int32 result;
int tot_backends = pgstat_fetch_stat_numbackends();
- int beid;+ int idx;
result = 0;
- for (beid = 1; beid <= tot_backends; beid++)+ for (idx = 1; idx <= tot_backends; idx++)
{
- LocalPgBackendStatus *local_beentry = pgstat_fetch_stat_local_beentry(beid);+ LocalPgBackendStatus *local_beentry = pgstat_get_local_beentry_by_index(idx);
if (local_beentry->backendStatus.st_databaseid == dbid)
result++;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.hindex 77939a0aed..1718ff7ce6 100644--- a/src/include/utils/backend_status.h+++ b/src/include/utils/backend_status.h@@ -333,8 +333,8 @@ extern uint64 pgstat_get_my_query_id(void);
* ----------
*/
extern int pgstat_fetch_stat_numbackends(void);
-extern PgBackendStatus *pgstat_fetch_stat_beentry(BackendId beid);-extern LocalPgBackendStatus *pgstat_fetch_stat_local_beentry(int beid);+extern PgBackendStatus *pgstat_get_beentry_by_backend_id(BackendId beid);+extern LocalPgBackendStatus *pgstat_get_local_beentry_by_index(int idx);
extern char *pgstat_clip_activity(const char *raw_activity);
--
2.25.1
v4-0002-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch (text/x-diff, 3.3K) download
From dc162a99a94e7c5483bfb77c40dac877e0ed5682 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 24 Aug 2023 07:58:01 -0700
Subject: [PATCH v4 2/2] fix pg_stat_get_backend_subxact to use real backend id
---
src/backend/utils/activity/backend_status.c | 36 +++++++++++++++------
src/backend/utils/adt/pgstatfuncs.c | 2 +-
src/include/utils/backend_status.h | 1 +
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.cindex a4860b10fc..722c5acf38 100644--- a/src/backend/utils/activity/backend_status.c+++ b/src/backend/utils/activity/backend_status.c@@ -1088,9 +1088,33 @@ cmp_lbestatus(const void *a, const void *b)
*/
PgBackendStatus *
pgstat_get_beentry_by_backend_id(BackendId beid)
+{+ LocalPgBackendStatus *ret = pgstat_get_local_beentry_by_backend_id(beid);++ if (ret)+ return &ret->backendStatus;++ return NULL;+}+++/* ----------+ * pgstat_get_local_beentry_by_backend_id() -+ *+ * Like pgstat_get_beentry_by_backend_id() but with locally computed additions+ * (like xid and xmin values of the backend)+ *+ * The beid argument is the BackendId of the desired session+ * (note that this is unlike pgstat_get_local_beentry_by_index()).+ *+ * NB: caller is responsible for checking if the user is permitted to see this+ * info (especially the querystring).+ * ----------+ */+LocalPgBackendStatus *+pgstat_get_local_beentry_by_backend_id(BackendId beid)
{
LocalPgBackendStatus key;
- LocalPgBackendStatus *ret;
pgstat_read_current_status();
@@ -1099,14 +1123,8 @@ pgstat_get_beentry_by_backend_id(BackendId beid)
* bsearch() to search it efficiently.
*/
key.backend_id = beid;
- ret = (LocalPgBackendStatus *) bsearch(&key, localBackendStatusTable,- localNumBackends,- sizeof(LocalPgBackendStatus),- cmp_lbestatus);- if (ret)- return &ret->backendStatus;-- return NULL;+ return bsearch(&key, localBackendStatusTable, localNumBackends,+ sizeof(LocalPgBackendStatus), cmp_lbestatus);
}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.cindex 49cc887b97..dd5094a2d4 100644--- a/src/backend/utils/adt/pgstatfuncs.c+++ b/src/backend/utils/adt/pgstatfuncs.c@@ -727,7 +727,7 @@ pg_stat_get_backend_subxact(PG_FUNCTION_ARGS)
BlessTupleDesc(tupdesc);
- if ((local_beentry = pgstat_get_local_beentry_by_index(beid)) != NULL)+ if ((local_beentry = pgstat_get_local_beentry_by_backend_id(beid)) != NULL)
{
/* Fill values and NULLs */
values[0] = Int32GetDatum(local_beentry->backend_subxact_count);
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.hindex 1718ff7ce6..d51c840daf 100644--- a/src/include/utils/backend_status.h+++ b/src/include/utils/backend_status.h@@ -334,6 +334,7 @@ extern uint64 pgstat_get_my_query_id(void);
*/
extern int pgstat_fetch_stat_numbackends(void);
extern PgBackendStatus *pgstat_get_beentry_by_backend_id(BackendId beid);
+extern LocalPgBackendStatus *pgstat_get_local_beentry_by_backend_id(BackendId beid);
extern LocalPgBackendStatus *pgstat_get_local_beentry_by_index(int idx);
extern char *pgstat_clip_activity(const char *raw_activity);
--
2.25.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, michael@paquier.xyz, simseih@amazon.com, barwick@gmail.com, robertmhaas@gmail.com
Subject: Re: pg_stat_get_backend_subxact() and backend IDs?
In-Reply-To: <20230830020151.GA2488217@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox