Subject: Re: pg_stat_get_backend_subxact() and backend IDs?

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 09:46:55 -0700

On Mon, Aug 28, 2023 at 10:53:52AM +0900, Michael Paquier wrote:
> 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.

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.

[0] https://postgr.es/m/20230824161913.GA1394441%40nathanxps13.lan

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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 30 Aug 2023 08:22:27 +0900

> 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.

+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.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 19:01:51 -0700

> released to get things in a better shape while addressing the issue

Okay.

> refers to an index ID, and not a backend ID, especially for the

Makes sense.  I did this in v4.

>From 3e360c1b970526079528159fe1c49be03d0b13b5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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.c
index 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.c
index 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.h
index 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

>From dc162a99a94e7c5483bfb77c40dac877e0ed5682 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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.c
index 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.c
index 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.h
index 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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 30 Aug 2023 13:13:31 +0900

> Makes sense.  I did this in v4.

Yep, that looks more consistent, at quick glance.
--
Michael

---

From: Robert Haas <[email-masked]>
Date: Wed, 30 Aug 2023 09:50:41 -0400

> Yep, that looks more consistent, at quick glance.

Sorry, I'm only just noticing this thread. Thanks, Nathan, Ian, and
others, for your work on this. Apart from hoping that the 0002 patch
will get a more detailed commit message spelling out the problem very
explicitly, I don't have any comments on the proposed patches.

--
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 30 Aug 2023 07:27:55 -0700

> explicitly, I don't have any comments on the proposed patches.

I'm about to spend way too much time writing the commit message for 0002,
but I plan to commit both patches sometime today.

---

From: Robert Haas <[email-masked]>
Date: Wed, 30 Aug 2023 10:56:22 -0400

> but I plan to commit both patches sometime today.

Thanks! I'm glad your committing the patches, and I approve of you
spending way too much time on the commit message. :-)

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 30 Aug 2023 14:56:48 -0700

> spending way too much time on the commit message. :-)

Committed.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 31 Aug 2023 13:22:53 +0900

> Committed.

Cool, thanks!
--
Michael

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 31 Aug 2023 16:47:21 +0530

> > spending way too much time on the commit message. :-)

Sorry, I didn't notice this thread earlier.  The new behavior looks
better to me, thanks for working on it.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Ian Lawrence Barwick <[email-masked]>
Date: Mon, 18 Sep 2023 15:52:53 +0900

> > spending way too much time on the commit message. :-)

Thanks for taking care of this (saw the commits, then got distracted
by something else and forgot to follow up).

Regards

Ian Barwick