public inbox for [email protected]
help / color / mirror / Atom feedRe: pg_stat_get_backend_subxact() and backend IDs?
10+ messages / 5 participants
[nested] [flat]
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-24 02:51 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Bossart @ 2023-08-24 02:51 UTC (permalink / raw)
To: Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; [email protected]
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-24 16:19 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Bossart @ 2023-08-24 16:19 UTC (permalink / raw)
To: Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; [email protected]
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
Attachments:
[text/x-diff] v2-0001-rename-some-pgstat-functions.patch (9.7K, ../../[email protected]/2-v2-0001-rename-some-pgstat-functions.patch)
download | inline diff:
From 3af12242ec10abe83d195e4b3f0765afec40f710 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
[text/x-diff] v2-0002-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch (3.3K, ../../[email protected]/3-v2-0002-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch)
download | inline diff:
From 82d95d01526d09c565413696a366b350a9f796ff Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 00:36 Ian Lawrence Barwick <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: Ian Lawrence Barwick @ 2023-08-25 00:36 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers; [email protected]
2023年8月25日(金) 1:19 Nathan Bossart <[email protected]>:
>
> 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?
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 15:01 Imseih (AWS), Sami <[email protected]>
parent: Ian Lawrence Barwick <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Imseih (AWS), Sami @ 2023-08-25 15:01 UTC (permalink / raw)
To: Ian Lawrence Barwick <[email protected]>; Nathan Bossart <[email protected]>; +Cc: pgsql-hackers; [email protected] <[email protected]>
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
+ return bsearch(&key, localBackendStatusTable, localNumBackends,
+ sizeof(LocalPgBackendStatus), cmp_lbestatus);
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)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 15:32 Nathan Bossart <[email protected]>
parent: Ian Lawrence Barwick <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Nathan Bossart @ 2023-08-25 15:32 UTC (permalink / raw)
To: Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; [email protected]
On Fri, Aug 25, 2023 at 09:36:18AM +0900, Ian Lawrence Barwick wrote:
> 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.
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...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 15:36 Nathan Bossart <[email protected]>
parent: Imseih (AWS), Sami <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2023-08-25 15:36 UTC (permalink / raw)
To: Imseih (AWS), Sami <[email protected]>; +Cc: Ian Lawrence Barwick <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
On Fri, Aug 25, 2023 at 03:01:40PM +0000, Imseih (AWS), Sami wrote:
> 1/ cast the return of bsearch. This was done previously and is the common
> convention in the code.
Will do.
> 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].
I'd rather keep this patch focused on fixing the bug, given we are so close
to the v16 release.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 19:29 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Bossart @ 2023-08-25 19:29 UTC (permalink / raw)
To: Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; [email protected]
On Fri, Aug 25, 2023 at 08:32:51AM -0700, Nathan Bossart wrote:
> 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...
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch (3.3K, ../../20230825192949.GA1917227@nathanxps13/2-v3-0001-fix-pg_stat_get_backend_subxact-to-use-real-backe.patch)
download | inline diff:
From e1ce8bf67f1b713294ef8d38dbee26bfc7ef16f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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);
}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 2b9742ad21..68ae044399 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_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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-25 22:56 Imseih (AWS), Sami <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Imseih (AWS), Sami @ 2023-08-25 22:56 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Ian Lawrence Barwick <[email protected]>; +Cc: pgsql-hackers; [email protected] <[email protected]>
> 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.
Thanks for the updated patch.
I reviewed/tested the latest version and I don't have any more comments.
Regards,
Sami
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pg_stat_get_backend_subxact() and backend IDs?
@ 2023-08-28 01:53 Michael Paquier <[email protected]>
parent: Imseih (AWS), Sami <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2023-08-28 01:53 UTC (permalink / raw)
To: Imseih (AWS), Sami <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Ian Lawrence Barwick <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
On Fri, Aug 25, 2023 at 10:56:14PM +0000, Imseih (AWS), Sami wrote:
> > 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.
>
> Thanks for the updated patch.
>
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZOv+MHonWVgW%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH v1 6/8] convert SharedFileSet->refcnt to an atomic
@ 2026-07-09 19:53 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2026-07-09 19:53 UTC (permalink / raw)
---
src/backend/storage/file/sharedfileset.c | 27 +++++++++---------------
src/include/storage/sharedfileset.h | 5 ++---
2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/src/backend/storage/file/sharedfileset.c b/src/backend/storage/file/sharedfileset.c
index d76bd72dc63..4f12f92beae 100644
--- a/src/backend/storage/file/sharedfileset.c
+++ b/src/backend/storage/file/sharedfileset.c
@@ -38,8 +38,7 @@ void
SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg)
{
/* Initialize the shared fileset specific members. */
- SpinLockInit(&fileset->mutex);
- fileset->refcnt = 1;
+ pg_atomic_init_u32(&fileset->refcnt, 1);
/* Initialize the fileset. */
FileSetInit(&fileset->fs);
@@ -55,19 +54,15 @@ SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg)
void
SharedFileSetAttach(SharedFileSet *fileset, dsm_segment *seg)
{
- bool success;
+ uint32 refcnt;
- SpinLockAcquire(&fileset->mutex);
- if (fileset->refcnt == 0)
- success = false;
- else
- {
- ++fileset->refcnt;
- success = true;
- }
- SpinLockRelease(&fileset->mutex);
+ refcnt = pg_atomic_read_u32(&fileset->refcnt);
+ while (refcnt != 0 &&
+ !pg_atomic_compare_exchange_u32(&fileset->refcnt, &refcnt,
+ refcnt + 1))
+ ;
- if (!success)
+ if (refcnt == 0)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("could not attach to a SharedFileSet that is already destroyed")));
@@ -98,11 +93,9 @@ SharedFileSetOnDetach(dsm_segment *segment, Datum datum)
bool unlink_all = false;
SharedFileSet *fileset = (SharedFileSet *) DatumGetPointer(datum);
- SpinLockAcquire(&fileset->mutex);
- Assert(fileset->refcnt > 0);
- if (--fileset->refcnt == 0)
+ Assert(pg_atomic_read_u32(&fileset->refcnt) > 0);
+ if (pg_atomic_sub_fetch_u32(&fileset->refcnt, 1) == 0)
unlink_all = true;
- SpinLockRelease(&fileset->mutex);
/*
* If we are the last to detach, we delete the directory in all
diff --git a/src/include/storage/sharedfileset.h b/src/include/storage/sharedfileset.h
index 904396e7173..d89626ae64b 100644
--- a/src/include/storage/sharedfileset.h
+++ b/src/include/storage/sharedfileset.h
@@ -15,10 +15,10 @@
#ifndef SHAREDFILESET_H
#define SHAREDFILESET_H
+#include "port/atomics.h"
#include "storage/dsm.h"
#include "storage/fd.h"
#include "storage/fileset.h"
-#include "storage/spin.h"
/*
* A set of temporary files that can be shared by multiple backends.
@@ -26,8 +26,7 @@
typedef struct SharedFileSet
{
FileSet fs;
- slock_t mutex; /* mutex protecting the reference count */
- int refcnt; /* number of attached backends */
+ pg_atomic_uint32 refcnt; /* number of attached backends */
} SharedFileSet;
extern void SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg);
--
2.50.1 (Apple Git-155)
--Ot5x3ffkWEuxilWO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v1-0007-convert-ParallelBlockTableScanDescData-phs_-start.patch
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-07-09 19:53 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-08-24 02:51 Re: pg_stat_get_backend_subxact() and backend IDs? Nathan Bossart <[email protected]>
2023-08-24 16:19 ` Nathan Bossart <[email protected]>
2023-08-25 00:36 ` Ian Lawrence Barwick <[email protected]>
2023-08-25 15:01 ` Imseih (AWS), Sami <[email protected]>
2023-08-25 15:36 ` Nathan Bossart <[email protected]>
2023-08-25 15:32 ` Nathan Bossart <[email protected]>
2023-08-25 19:29 ` Nathan Bossart <[email protected]>
2023-08-25 22:56 ` Imseih (AWS), Sami <[email protected]>
2023-08-28 01:53 ` Michael Paquier <[email protected]>
2026-07-09 19:53 [PATCH v1 6/8] convert SharedFileSet->refcnt to an atomic Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox