Subject: Re: pg_stat_get_backend_subxact() and backend IDs?

From: Nathan Bossart <[email-masked]>
Date: Wed, 23 Aug 2023 19:51:40 -0700

On Wed, Aug 23, 2023 at 07:32:06PM -0700, Nathan Bossart wrote:
> On Thu, Aug 24, 2023 at 10:22:49AM +0900, Ian Lawrence Barwick wrote:
>> Looking at the code, this is happening because
>> "pgstat_fetch_stat_local_beentry()"
>> expects to be passed the backend ID as an integer representing a 1-based index
>> referring to "localBackendStatusTable", but "pg_stat_get_backend_subxact()"
>> is presumably intended to take the actual BackendId , as per other
>> "pg_stat_get_XXX()"
>> functions.
> 
> Yes, this was changed in d7e39d7, but 10ea0f9 seems to have missed the
> memo.

BTW I'd argue that this is a bug in v16 that we should try to fix before
GA, so I've added an open item [0].  I assigned it to Robert (CC'd) since
he was the committer, but I'm happy to pick it up.

[0] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items#Open_Issues

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Aug 2023 09:19:13 -0700

> he was the committer, but I'm happy to pick it up.

Since RC1 is fast approaching, I put together a revised patch set.  0001
renames the existing pgstat_fetch_stat* functions, and 0002 adds
pgstat_get_local_beentry_by_backend_id() and uses it for
pg_stat_get_backend_subxact().  Thoughts?

>From 3af12242ec10abe83d195e4b3f0765afec40f710 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Aug 2023 07:42:54 -0700
Subject: [PATCH v2 1/2] rename some pgstat functions

---
 src/backend/utils/activity/backend_status.c | 22 +++++++-------
 src/backend/utils/adt/pgstatfuncs.c         | 32 ++++++++++-----------
 src/include/utils/backend_status.h          |  4 +--
 3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 38f91a495b..aea05a9014 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()).
+ *	(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,7 +1125,7 @@ pgstat_fetch_stat_beentry(BackendId beid)
  * ----------
  */
 LocalPgBackendStatus *
-pgstat_fetch_stat_local_beentry(int beid)
+pgstat_get_local_beentry_by_index(int beid)
 {
 	pgstat_read_current_status();
 
@@ -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.c
index 2b9742ad21..7deefed5ab 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))
@@ -983,7 +983,7 @@ pg_stat_get_db_numbackends(PG_FUNCTION_ARGS)
 	result = 0;
 	for (beid = 1; beid <= tot_backends; beid++)
 	{
-		LocalPgBackendStatus *local_beentry = pgstat_fetch_stat_local_beentry(beid);
+		LocalPgBackendStatus *local_beentry = pgstat_get_local_beentry_by_index(beid);
 
 		if (local_beentry->backendStatus.st_databaseid == dbid)
 			result++;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 77939a0aed..e53098ec9e 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 beid);
 extern char *pgstat_clip_activity(const char *raw_activity);

-- 
2.25.1

>From 82d95d01526d09c565413696a366b350a9f796ff Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Aug 2023 07:58:01 -0700
Subject: [PATCH v2 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.c
index aea05a9014..d5babd9fb0 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.c
index 7deefed5ab..62d327bb92 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.h
index e53098ec9e..a996c7fd23 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 beid);
 extern char *pgstat_clip_activity(const char *raw_activity);
 
-- 
2.25.1

---

From: Ian Lawrence Barwick <[email-masked]>
Date: Fri, 25 Aug 2023 09:36:18 +0900

> pg_stat_get_backend_subxact().  Thoughts?

Thanks for looking at this. In summary we now have these functions:

 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 beid);

which LGTM; patches work as expected and resolve the reported issue.

Regards

Ian Barwick

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Fri, 25 Aug 2023 15:01:40 +0000

I tested the patch and it does the correct thing.

I have a few comments:

1/ cast the return of bsearch. This was done previously and is the common
convention in the code.

So

Should be

+       return (LocalPgBackendStatus *) bsearch(&key, localBackendStatusTable, localNumBackends,
+                                  sizeof(LocalPgBackendStatus), cmp_lbestatus);

2/ This will probably be a good time to update the docs for pg_stat_get_backend_subxact [1]
to call out that "subxact_count" will "only increase if a transaction is performing writes". Also to link
the reader to the subtransactions doc [2].

1. https://www.postgresql.org/docs/16/monitoring-stats.html#WAIT-EVENT-TIMEOUT-TABLE
2. https://www.postgresql.org/docs/16/subxacts.html

Regards,

Sami Imseih
Amazon Web Services (AWS)

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 25 Aug 2023 08:32:51 -0700

> which LGTM; patches work as expected and resolve the reported issue.

On second thought, renaming these exported functions so close to release is
probably not a great idea.  I should probably skip back-patching that one.
Or I could have the existing functions call the new ones in v16 for
backward compatibility...

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 25 Aug 2023 08:36:21 -0700

> 1/ cast the return of bsearch. This was done previously and is the common

Will do.

> the reader to the subtransactions doc [2].

I'd rather keep this patch focused on fixing the bug, given we are so close
to the v16 release.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 25 Aug 2023 12:29:49 -0700

> backward compatibility...

Here is a new version of the patch that avoids changing the names of the
existing functions.  I'm not thrilled about the name
(pgstat_fetch_stat_local_beentry_by_backend_id), so I am open to
suggestions.  In any case, I'd like to rename all three of the
pgstat_fetch_stat_* functions in v17.

>From e1ce8bf67f1b713294ef8d38dbee26bfc7ef16f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 24 Aug 2023 07:58:01 -0700
Subject: [PATCH v3 1/1] fix pg_stat_get_backend_subxact to use real backend id

---
 src/backend/utils/activity/backend_status.c | 39 ++++++++++++++++-----
 src/backend/utils/adt/pgstatfuncs.c         |  2 +-
 src/include/utils/backend_status.h          |  1 +
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 38f91a495b..231cc5dd9a 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -1089,9 +1089,34 @@ cmp_lbestatus(const void *a, const void *b)
 PgBackendStatus *
 pgstat_fetch_stat_beentry(BackendId beid)
 {
-	LocalPgBackendStatus key;
 	LocalPgBackendStatus *ret;
 
+	ret = pgstat_fetch_stat_local_beentry_by_backend_id(beid);
+	if (ret)
+		return &ret->backendStatus;
+
+	return NULL;
+}
+
+
+/* ----------
+ * pgstat_fetch_stat_local_beentry_by_backend_id() -
+ *
+ *	Like pgstat_fetch_stat_beentry() 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_fetch_stat_local_beentry()).
+ *
+ *	NB: caller is responsible for checking if the user is permitted to see this
+ *	info (especially the querystring).
+ * ----------
+ */
+LocalPgBackendStatus *
+pgstat_fetch_stat_local_beentry_by_backend_id(BackendId beid)
+{
+	LocalPgBackendStatus key;
+
 	pgstat_read_current_status();
 
 	/*
@@ -1099,14 +1124,10 @@ pgstat_fetch_stat_beentry(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 (LocalPgBackendStatus *) bsearch(&key, localBackendStatusTable,
+											localNumBackends,
+											sizeof(LocalPgBackendStatus),
+											cmp_lbestatus);
 }

index 2b9742ad21..68ae044399 100644
 
 	BlessTupleDesc(tupdesc);
 
-	if ((local_beentry = pgstat_fetch_stat_local_beentry(beid)) != NULL)
+	if ((local_beentry = pgstat_fetch_stat_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.h
index 77939a0aed..060431a39b 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -335,6 +335,7 @@ 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 LocalPgBackendStatus *pgstat_fetch_stat_local_beentry_by_backend_id(BackendId beid);
 extern char *pgstat_clip_activity(const char *raw_activity);

-- 
2.25.1

---

From: "Imseih (AWS), Sami" <[email-masked]>
Date: Fri, 25 Aug 2023 22:56:14 +0000

> suggestions. In any case, I'd like to rename all three of the>

Thanks for the updated patch.

I reviewed/tested the latest version and I don't have any more comments.

Regards,

Sami

---

From: Michael Paquier <[email-masked]>
Date: Mon, 28 Aug 2023 10:53:52 +0900

> I reviewed/tested the latest version and I don't have any more comments.

FWIW, I find the new routine introduced by this patch rather
confusing.  pgstat_fetch_stat_local_beentry() and
pgstat_fetch_stat_local_beentry_by_backend_id() use the same 
argument name for a BackendId or an int.  This is not entirely the
fault of this patch as pg_stat_get_backend_subxact() itself is
confused about "beid" being a uint32 or a BackendId.  However, I think
that this makes much harder to figure out that
pgstat_fetch_stat_local_beentry() is only here because it is cheaper 
to do sequential scan of all the local beentries rather than a
bsearch() for all its callers, while
pgstat_fetch_stat_local_beentry_by_backend_id() is here because we
want to retrieve the local beentry matching with the *backend ID* with
the binary search().

I understand that this is not a fantastic naming, but renaming
pgstat_fetch_stat_local_beentry() to something like
pgstat_fetch_stat_local_beentry_by_{index|position}_id() would make
the difference much easier to grasp, and we should avoid the use of
"beid" when we refer to the *position/index ID* in
localBackendStatusTable, because it is not a BackendId at all, just a
position in the local array.
--
Michael