From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Greg Stark <stark@mit.edu>
Cc: pgsql-hackers@postgresql.org
Subject: Re: identifying the backend that owns a temporary schema
Date: Wed, 28 Sep 2022 18:56:20 -0400
Message-ID: <4157446.1664405780@sss.pgh.pa.us> (raw)
In-Reply-To: <20220926201113.GA1340606@nathanxps13>
References: <20220815205811.GA250990@nathanxps13>
<CAM-w4HMZ03AbQjBbfN_mTxct68HX3NPRQVfLaRxLGJTejJ8xbg@mail.gmail.com>
<20220829230757.GA542616@nathanxps13>
<2297420.1664041298@sss.pgh.pa.us>
<20220926160822.GA1298186@nathanxps13>
<3499642.1664221809@sss.pgh.pa.us>
<20220926201113.GA1340606@nathanxps13>
Nathan Bossart <nathandbossart@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
Attachments:v5-0001-use-real-backend-id.patch (text/x-diff, 13.2K) downloaddiff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgmlindex 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.cindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, nathandbossart@gmail.com, stark@mit.edu
Subject: Re: identifying the backend that owns a temporary schema
In-Reply-To: <4157446.1664405780@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox