Nathan Bossart <nathandboss...@gmail.com> writes:
> Thanks for the suggestion. I used it in v4 of the patch.
I reviewed this and made some changes, some cosmetic some less so.
Notably, I was bemused that of the four calls of
pgstat_fetch_stat_local_beentry, three tested for a NULL result even
though they cannot get one, while the fourth (pg_stat_get_backend_idset)
*is* at hazard of a NULL result but lacked a check. I changed
pg_stat_get_backend_idset so that it too cannot get a NULL, and deleted
the dead code from the other callers.
A point that still bothers me a bit about pg_stat_get_backend_idset is
that it could miss or duplicate some backend IDs if the user calls
pg_stat_clear_snapshot() partway through the SRF's run, and we reload
a different set of backend entries than we had before. I added a comment
about that, with an argument why it's not worth working harder, but
is the argument convincing? If not, what should we do?
Also, I realized that the functions we're changing here are mostly
not exercised in the current regression tests :-(. So I added a
small test case.
I think this is probably committable if you agree with my changes.
regards, tom lane
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1d9509a2f6..342b20ebeb 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5485,20 +5485,23 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
the <structname>pg_stat_activity</structname> view, returns a set of records
containing all the available information about each backend process.
Sometimes it may be more convenient to obtain just a subset of this
- information. In such cases, an older set of per-backend statistics
+ information. In such cases, another set of per-backend statistics
access functions can be used; these are shown in <xref
linkend="monitoring-stats-backend-funcs-table"/>.
- These access functions use a backend ID number, which ranges from one
- to the number of currently active backends.
+ These access functions use the session's backend ID number, which is a
+ small positive integer that is distinct from the backend ID of any
+ concurrent session, although a session's ID can be recycled as soon as
+ it exits. The backend ID is used, among other things, to identify the
+ session's temporary schema if it has one.
The function <function>pg_stat_get_backend_idset</function> provides a
- convenient way to generate one row for each active backend for
+ convenient way to list all the active backends' ID numbers for
invoking these functions. For example, to show the <acronym>PID</acronym>s and
current queries of all backends:
<programlisting>
-SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
- pg_stat_get_backend_activity(s.backendid) AS query
- FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
+SELECT pg_stat_get_backend_pid(backendid) AS pid,
+ pg_stat_get_backend_activity(backendid) AS query
+FROM pg_stat_get_backend_idset() AS backendid;
</programlisting>
</para>
@@ -5526,8 +5529,7 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
<returnvalue>setof integer</returnvalue>
</para>
<para>
- Returns the set of currently active backend ID numbers (from 1 to the
- number of active backends).
+ Returns the set of currently active backend ID numbers.
</para></entry>
</row>
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index c7ed1e6d7a..22c79e156b 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -846,6 +846,12 @@ pgstat_read_current_status(void)
/* Only valid entries get included into the local array */
if (localentry->backendStatus.st_procpid > 0)
{
+ /*
+ * The array index is exactly the BackendId of the source backend.
+ * Note that this means the localBackendStatusTable is in order by
+ * backend_id. pgstat_fetch_stat_beentry() depends on that.
+ */
+ localentry->backend_id = i;
BackendIdGetTransactionIds(i,
&localentry->backend_xid,
&localentry->backend_xmin);
@@ -1045,26 +1051,57 @@ pgstat_get_my_query_id(void)
return MyBEEntry->st_query_id;
}
+/* ----------
+ * cmp_lbestatus
+ *
+ * Comparison function for bsearch() on an array of LocalPgBackendStatus.
+ * The backend_id field is used to compare the arguments.
+ * ----------
+ */
+static int
+cmp_lbestatus(const void *a, const void *b)
+{
+ const LocalPgBackendStatus *lbestatus1 = (const LocalPgBackendStatus *) a;
+ const LocalPgBackendStatus *lbestatus2 = (const LocalPgBackendStatus *) b;
+
+ return lbestatus1->backend_id - lbestatus2->backend_id;
+}
/* ----------
* pgstat_fetch_stat_beentry() -
*
* Support function for the SQL-callable pgstat* functions. Returns
- * our local copy of the current-activity entry for one backend.
+ * 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()).
*
* NB: caller is responsible for a check if the user is permitted to see
* this info (especially the querystring).
* ----------
*/
PgBackendStatus *
-pgstat_fetch_stat_beentry(int beid)
+pgstat_fetch_stat_beentry(BackendId beid)
{
+ LocalPgBackendStatus key;
+ LocalPgBackendStatus *ret;
+
pgstat_read_current_status();
- if (beid < 1 || beid > localNumBackends)
- return NULL;
+ /*
+ * Since the localBackendStatusTable is in order by backend_id, we can use
+ * 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 &localBackendStatusTable[beid - 1].backendStatus;
+ return NULL;
}
@@ -1074,6 +1111,10 @@ pgstat_fetch_stat_beentry(int beid)
* Like pgstat_fetch_stat_beentry() 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()).
+ * 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
* this info (especially the querystring).
* ----------
@@ -1094,7 +1135,8 @@ pgstat_fetch_stat_local_beentry(int beid)
* pgstat_fetch_stat_numbackends() -
*
* Support function for the SQL-callable pgstat* functions. Returns
- * the maximum current backend id.
+ * the number of sessions known in the localBackendStatusTable, i.e.
+ * the maximum 1-based index to pass to pgstat_fetch_stat_local_beentry().
* ----------
*/
int
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index be15b4b2e5..d5ca9208d7 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -415,7 +415,6 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
{
FuncCallContext *funcctx;
int *fctx;
- int32 result;
/* stuff done only on the first call of the function */
if (SRF_IS_FIRSTCALL())
@@ -424,11 +423,10 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
funcctx = SRF_FIRSTCALL_INIT();
fctx = MemoryContextAlloc(funcctx->multi_call_memory_ctx,
- 2 * sizeof(int));
+ sizeof(int));
funcctx->user_fctx = fctx;
fctx[0] = 0;
- fctx[1] = pgstat_fetch_stat_numbackends();
}
/* stuff done on every call of the function */
@@ -436,12 +434,22 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
fctx = funcctx->user_fctx;
fctx[0] += 1;
- result = fctx[0];
- if (result <= fctx[1])
+ /*
+ * We recheck pgstat_fetch_stat_numbackends() each time through, just in
+ * case the local status data has been refreshed since we started. It's
+ * plenty cheap enough if not. If a refresh does happen, we'll likely
+ * miss or duplicate some backend IDs, but we're content not to crash.
+ * (Refreshing midway through such a query would be problematic usage
+ * anyway, since the other pgstatfuncs functions that the backend IDs will
+ * get passed to would then deliver inconsistent results.)
+ */
+ if (fctx[0] <= pgstat_fetch_stat_numbackends())
{
/* do when there is more left to send */
- SRF_RETURN_NEXT(funcctx, Int32GetDatum(result));
+ LocalPgBackendStatus *local_beentry = pgstat_fetch_stat_local_beentry(fctx[0]);
+
+ SRF_RETURN_NEXT(funcctx, Int32GetDatum(local_beentry->backend_id));
}
else
{
@@ -493,17 +501,13 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
int i;
local_beentry = pgstat_fetch_stat_local_beentry(curr_backend);
-
- if (!local_beentry)
- continue;
-
beentry = &local_beentry->backendStatus;
/*
* Report values for only those backends which are running the given
* command.
*/
- if (!beentry || beentry->st_progress_command != cmdtype)
+ if (beentry->st_progress_command != cmdtype)
continue;
/* Value available to all callers */
@@ -558,24 +562,6 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
/* Get the next one in the list */
local_beentry = pgstat_fetch_stat_local_beentry(curr_backend);
- if (!local_beentry)
- {
- int i;
-
- /* Ignore missing entries if looking for specific PID */
- if (pid != -1)
- continue;
-
- for (i = 0; i < lengthof(nulls); i++)
- nulls[i] = true;
-
- nulls[5] = false;
- values[5] = CStringGetTextDatum("<backend information not available>");
-
- tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
- continue;
- }
-
beentry = &local_beentry->backendStatus;
/* If looking for specific PID, ignore all the others */
@@ -1180,9 +1166,9 @@ pg_stat_get_db_numbackends(PG_FUNCTION_ARGS)
result = 0;
for (beid = 1; beid <= tot_backends; beid++)
{
- PgBackendStatus *beentry = pgstat_fetch_stat_beentry(beid);
+ LocalPgBackendStatus *local_beentry = pgstat_fetch_stat_local_beentry(beid);
- if (beentry && beentry->st_databaseid == dbid)
+ 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 7403bca25e..b582b46e9f 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -13,6 +13,7 @@
#include "datatype/timestamp.h"
#include "libpq/pqcomm.h"
#include "miscadmin.h" /* for BackendType */
+#include "storage/backendid.h"
#include "utils/backend_progress.h"
@@ -247,6 +248,13 @@ typedef struct LocalPgBackendStatus
*/
PgBackendStatus backendStatus;
+ /*
+ * The backend ID. For auxiliary processes, this will be set to a value
+ * greater than MaxBackends (since auxiliary processes do not have proper
+ * backend IDs).
+ */
+ BackendId backend_id;
+
/*
* The xid of the current transaction if available, InvalidTransactionId
* if not.
@@ -313,7 +321,7 @@ extern uint64 pgstat_get_my_query_id(void);
* ----------
*/
extern int pgstat_fetch_stat_numbackends(void);
-extern PgBackendStatus *pgstat_fetch_stat_beentry(int beid);
+extern PgBackendStatus *pgstat_fetch_stat_beentry(BackendId beid);
extern LocalPgBackendStatus *pgstat_fetch_stat_local_beentry(int beid);
extern char *pgstat_clip_activity(const char *raw_activity);
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 6a10dc462b..7a1ffd404d 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -576,15 +576,21 @@ SELECT sessions > :db_stat_sessions FROM pg_stat_database WHERE datname = (SELEC
-- Test pg_stat_bgwriter checkpointer-related stats, together with pg_stat_wal
SELECT checkpoints_req AS rqst_ckpts_before FROM pg_stat_bgwriter \gset
--- Test pg_stat_wal
+-- Test pg_stat_wal (and make a temp table so our temp schema exists)
SELECT wal_bytes AS wal_bytes_before FROM pg_stat_wal \gset
-CREATE TABLE test_stats_temp AS SELECT 17;
+CREATE TEMP TABLE test_stats_temp AS SELECT 17;
DROP TABLE test_stats_temp;
-- Checkpoint twice: The checkpointer reports stats after reporting completion
-- of the checkpoint. But after a second checkpoint we'll see at least the
-- results of the first.
CHECKPOINT;
CHECKPOINT;
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
SELECT checkpoints_req > :rqst_ckpts_before FROM pg_stat_bgwriter;
?column?
----------
@@ -597,6 +603,17 @@ SELECT wal_bytes > :wal_bytes_before FROM pg_stat_wal;
t
(1 row)
+-- Test pg_stat_get_backend_idset() and some allied functions.
+-- In particular, verify that their notion of backend ID matches
+-- our temp schema index.
+SELECT (current_schemas(true))[1] = ('pg_temp_' || beid::text) AS match
+FROM pg_stat_get_backend_idset() beid
+WHERE pg_stat_get_backend_pid(beid) = pg_backend_pid();
+ match
+-------
+ t
+(1 row)
+
-----
-- Test that resetting stats works for reset timestamp
-----
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index a6b0e9e042..197db3eba3 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -303,10 +303,10 @@ SELECT sessions > :db_stat_sessions FROM pg_stat_database WHERE datname = (SELEC
-- Test pg_stat_bgwriter checkpointer-related stats, together with pg_stat_wal
SELECT checkpoints_req AS rqst_ckpts_before FROM pg_stat_bgwriter \gset
--- Test pg_stat_wal
+-- Test pg_stat_wal (and make a temp table so our temp schema exists)
SELECT wal_bytes AS wal_bytes_before FROM pg_stat_wal \gset
-CREATE TABLE test_stats_temp AS SELECT 17;
+CREATE TEMP TABLE test_stats_temp AS SELECT 17;
DROP TABLE test_stats_temp;
-- Checkpoint twice: The checkpointer reports stats after reporting completion
@@ -315,9 +315,16 @@ DROP TABLE test_stats_temp;
CHECKPOINT;
CHECKPOINT;
+SELECT pg_stat_force_next_flush();
SELECT checkpoints_req > :rqst_ckpts_before FROM pg_stat_bgwriter;
SELECT wal_bytes > :wal_bytes_before FROM pg_stat_wal;
+-- Test pg_stat_get_backend_idset() and some allied functions.
+-- In particular, verify that their notion of backend ID matches
+-- our temp schema index.
+SELECT (current_schemas(true))[1] = ('pg_temp_' || beid::text) AS match
+FROM pg_stat_get_backend_idset() beid
+WHERE pg_stat_get_backend_pid(beid) = pg_backend_pid();
-----
-- Test that resetting stats works for reset timestamp
