Date: Thu, 24 Aug 2023 09:19:13 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Ian Lawrence Barwick <barwick@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, robertmhaas@gmail.com
Subject: Re: pg_stat_get_backend_subxact() and backend IDs?
Message-ID: <20230824161913.GA1394441@nathanxps13.lan>
References: <CAB8KJ=j-ACb3H4L9a_b3ZG3iCYDW5aEu3WsPAzkm2S7JzS1Few@mail.gmail.com>
 <20230824023206.GA1128857@nathanxps13.lan>
 <20230824025140.GB1128857@nathanxps13.lan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="gKMricLos+KVdGMg"
Content-Disposition: inline
In-Reply-To: <20230824025140.GB1128857@nathanxps13.lan>
Archived-At: <https://www.postgresql.org/message-id/20230824161913.GA1394441%40nathanxps13.lan>
Precedence: bulk


--gKMricLos+KVdGMg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Aug 23, 2023 at 07:51:40PM -0700, Nathan Bossart wrote:
> 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.

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?

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

--gKMricLos+KVdGMg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-rename-some-pgstat-functions.patch"

From 3af12242ec10abe83d195e4b3f0765afec40f710 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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


--gKMricLos+KVdGMg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0002-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch"

From 82d95d01526d09c565413696a366b350a9f796ff Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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


--gKMricLos+KVdGMg--