Thread: identifying the backend that owns a temporary schema

Hi hackers,

As Greg Stark noted elsewhere [0], it is presently very difficult to
identify the PID of the session using a temporary schema, which is
particularly unfortunate when a temporary table is putting a cluster in
danger of transaction ID wraparound.  I noted [1] that the following query
can be used to identify the PID for a given backend ID:

    SELECT bid, pg_stat_get_backend_pid(bid) AS pid FROM pg_stat_get_backend_idset() bid;

But on closer inspection, this is just plain wrong.  The backend IDs
returned by pg_stat_get_backend_idset() might initially bear some
resemblance to the backend IDs stored in PGPROC, so my suggested query
might work some of the time, but the pg_stat_get_backend_* backend IDs
typically diverge from the PGPROC backend IDs as sessions connect and
disconnect.

I think it would be nice to have a reliable way to discover the PID for a
given temporary schema via SQL.  The other thread [2] introduces a helpful
log message that indicates the PID for temporary tables that are in danger
of causing transaction ID wraparound, and I intend for this proposal to be
complementary to that work.

At first, I thought about adding a new function for retrieving the PGPROC
backend IDs, but I am worried that having two sets of backend IDs would be
even more confusing than having one set that can't reliably be used for
temporary schemas.  Instead, I tried adjusting the pg_stat_get_backend_*()
suite of functions to use the PGPROC backend IDs.  This ended up being
simpler than anticipated.  I added a backend_id field to the
LocalPgBackendStatus struct (which is populated within
pgstat_read_current_status()), and I changed pgstat_fetch_stat_beentry() to
bsearch() for the entry with the given PGPROC backend ID.

This does result in a small behavior change.  Currently,
pg_stat_get_backend_idset() simply returns a range of numbers (1 to the
number of active backends).  With the attached patch, this function will
still return a set of numbers, but there might be gaps between the IDs, and
the maximum backend ID will usually be greater than the number of active
backends.  I suppose this might break some existing uses, but I'm not sure
how much we should worry about that.  IMO uniting the backend IDs is a net
improvement.

Thoughts?

[0] https://postgr.es/m/CAM-w4HPCOuJDs4fdkgNdA8FFMeYMULPCAxjPpsOgvCO24KOAVg%40mail.gmail.com
[1] https://postgr.es/m/DDF0D1BC-261D-45C2-961C-5CBDBB41EE71%40amazon.com
[2] https://commitfest.postgresql.org/39/3358/

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

On 8/15/22 1:58 PM, Nathan Bossart wrote:
> Hi hackers,
> 
> As Greg Stark noted elsewhere [0], it is presently very difficult to
> identify the PID of the session using a temporary schema, which is
> particularly unfortunate when a temporary table is putting a cluster in
> danger of transaction ID wraparound.  I noted [1] that the following query
> can be used to identify the PID for a given backend ID:
> 
>     SELECT bid, pg_stat_get_backend_pid(bid) AS pid FROM pg_stat_get_backend_idset() bid;
> 
> But on closer inspection, this is just plain wrong.  The backend IDs
> returned by pg_stat_get_backend_idset() might initially bear some
> resemblance to the backend IDs stored in PGPROC, so my suggested query
> might work some of the time, but the pg_stat_get_backend_* backend IDs
> typically diverge from the PGPROC backend IDs as sessions connect and
> disconnect.

I didn't review the patch itself yet, but I'd like to chime in with a
big "+1" for the idea. I've had several past experiences getting called
to help in situations where a database was getting close to wraparound
and the culprit was a temp table blocking vacuum. I went down this same
trail of pg_stat_get_backend_idset() and I can attest that it did work
once or twice, but it didn't work other times.

AFAIK, in PostgreSQL today, there's really no way to reliably get the
PID of the session holding particular temp tables. (The idea of
iterating through backends with gdb and trying to find & dump some
obscure data structure seems completely impractical for regular
production ops.)

I'll take a look at the patch if I can... and I'm hopeful that we're
able to move this idea forward and get this little gap in PG filled once
and for all!

-Jeremy


-- 
Jeremy Schneider
Database Engineer
Amazon Web Services

On Mon, Aug 15, 2022 at 02:47:25PM -0700, Jeremy Schneider wrote:
> I'll take a look at the patch if I can... and I'm hopeful that we're
> able to move this idea forward and get this little gap in PG filled once
> and for all!

Thanks!

I noticed that the "result" variable in pg_stat_get_backend_idset() is kind
of pointless after my patch is applied, so here is a v2 with it removed.

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

At Tue, 16 Aug 2022 16:04:23 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Mon, Aug 15, 2022 at 02:47:25PM -0700, Jeremy Schneider wrote:
> > I'll take a look at the patch if I can... and I'm hopeful that we're
> > able to move this idea forward and get this little gap in PG filled once
> > and for all!
> 
> Thanks!
> 
> I noticed that the "result" variable in pg_stat_get_backend_idset() is kind
> of pointless after my patch is applied, so here is a v2 with it removed.

It seems to be a sensible way to expose the PGPROC backend ids to SQL
interface.  It inserts bsearch into relatively frequently-called
function but (I believe) that doesn't seem to matter much (comparing
to, for example, the size of id->position translation table).

I don't think pgstat_fetch_stat_beentry needs to check for
out-of-range ids anymore. That case is a kind of rare and bsearch
properly handles it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Having this function would be great (I admit I never responded because
I never figured out if your suggestion was right or not:). But should
it also be added to the pg_stat_activity view? Perhaps even just in
the SQL view using the function.

Alternately should pg_stat_activity show the actual temp schema name
instead of the id? I don't recall if it's visible outside the backend
but if it is, could pg_stat_activity show whether the temp schema is
actually attached or not?

On Tue, Aug 23, 2022 at 10:29:05AM +0100, Greg Stark wrote:
> Having this function would be great (I admit I never responded because
> I never figured out if your suggestion was right or not:). But should
> it also be added to the pg_stat_activity view? Perhaps even just in
> the SQL view using the function.
> 
> Alternately should pg_stat_activity show the actual temp schema name
> instead of the id? I don't recall if it's visible outside the backend
> but if it is, could pg_stat_activity show whether the temp schema is
> actually attached or not?

I'm open to adding the backend ID or the temp schema name to
pg_stat_activity, but I wouldn't be surprised to learn that others aren't.
It'd be great to hear a few more opinions on the idea before I spend too
much time on the patches.  IMO we should still adjust the
pg_stat_get_backend_*() functions even if we do end up adjusting
pg_stat_activity.

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

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Tue, Aug 23, 2022 at 10:29:05AM +0100, Greg Stark wrote:
>> Alternately should pg_stat_activity show the actual temp schema name
>> instead of the id? I don't recall if it's visible outside the backend
>> but if it is, could pg_stat_activity show whether the temp schema is
>> actually attached or not?

> I'm open to adding the backend ID or the temp schema name to
> pg_stat_activity, but I wouldn't be surprised to learn that others aren't.

FWIW, I'd vote against adding the temp schema per se.  We can see from
outside whether the corresponding temp schema exists, but we can't readily
tell whether the session has decided to use it, so attributing it to the
session is a bit dangerous.  Maybe there is an argument for having
sessions report it to pgstats when they do adopt a temp schema, but I
think there'd be race conditions, rollback after error, and other issues
to contend with there.

The proposed patch seems like an independent first step in any case.

One thing I don't like about it documentation-wise is that it leaves
the concept of backend ID pretty much completely undefined.

            regards, tom lane

On Sat, Sep 24, 2022 at 01:41:38PM -0400, Tom Lane wrote:
> One thing I don't like about it documentation-wise is that it leaves
> the concept of backend ID pretty much completely undefined.

How specific do you think this definition ought to be?  All I've come up
with so far is "internal identifier for the backend that is independent
from its PID," which is what I used in the attached patch.  Do we want to
mention its uses in more detail (e.g., temp schema name), or should we keep
it vague?

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

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sat, Sep 24, 2022 at 01:41:38PM -0400, Tom Lane wrote:
>> One thing I don't like about it documentation-wise is that it leaves
>> the concept of backend ID pretty much completely undefined.

> How specific do you think this definition ought to be?

Fairly specific, I think, so that people can reason about how it behaves.
Notably, it seems absolutely critical to be clear that the IDs recycle
over short time frames.  Maybe like

    These access functions use the session's backend ID number, which is
    a small integer that is distinct from the backend ID of any concurrent
    session, although an ID can be recycled as soon as the session exits.
    The backend ID is used, among other things, to identify the session's
    temporary schema if it has one.

I'd prefer to use the terminology "session" than "backend" in the
definition.  I suppose we can't get away with actually calling it
a "session ID" given that "backend ID" is used in so many places;
but I think people have a clearer handle on what a session is.

            regards, tom lane

On Mon, Sep 26, 2022 at 03:50:09PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Sat, Sep 24, 2022 at 01:41:38PM -0400, Tom Lane wrote:
>>> One thing I don't like about it documentation-wise is that it leaves
>>> the concept of backend ID pretty much completely undefined.
> 
>> How specific do you think this definition ought to be?
> 
> Fairly specific, I think, so that people can reason about how it behaves.
> Notably, it seems absolutely critical to be clear that the IDs recycle
> over short time frames.  Maybe like
> 
>     These access functions use the session's backend ID number, which is
>     a small integer that is distinct from the backend ID of any concurrent
>     session, although an ID can be recycled as soon as the session exits.
>     The backend ID is used, among other things, to identify the session's
>     temporary schema if it has one.
> 
> I'd prefer to use the terminology "session" than "backend" in the
> definition.  I suppose we can't get away with actually calling it
> a "session ID" given that "backend ID" is used in so many places;
> but I think people have a clearer handle on what a session is.

Thanks for the suggestion.  I used it in v4 of the patch.

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

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

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

On Wed, Sep 28, 2022 at 06:56:20PM -0400, Tom Lane wrote:
> I reviewed this and made some changes, some cosmetic some less so.

Thanks for the detailed review.

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

Isn't this an existing problem?  Granted, it'd manifest differently with
this patch, but ISTM we could already end up with too many or too few
backend IDs if there's a refresh partway through.  I don't know if there's
an easy way to avoid mismatches altogether besides perhaps ERROR-ing if
there's a concurrent refresh.

> -    if (beid < 1 || beid > localNumBackends)
> -        return NULL;

The reason I'd kept this in was because I was worried about overflow in the
comparator function.  Upon further inspection, I don't think there's
actually any way that will produce incorrect results.  And I'm not sure we
should worry about such cases too much, anyway.

Overall, LGTM.

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

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Sep 28, 2022 at 06:56:20PM -0400, Tom Lane wrote:
>> 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?

> Isn't this an existing problem?  Granted, it'd manifest differently with
> this patch, but ISTM we could already end up with too many or too few
> backend IDs if there's a refresh partway through.

Right.  I'd been thinking the current code wouldn't generate duplicate IDs
--- but it might produce duplicate query output anyway, in case a given
backend's entry is later in the array than it was before.  So really
there's not much guarantees here in any case.

>> -    if (beid < 1 || beid > localNumBackends)
>> -        return NULL;

> The reason I'd kept this in was because I was worried about overflow in the
> comparator function.  Upon further inspection, I don't think there's
> actually any way that will produce incorrect results.  And I'm not sure we
> should worry about such cases too much, anyway.

Ah, I see: if the user passes in a "backend ID" that is close to INT_MIN,
then the comparator's subtraction could overflow.  We could fix that by
writing out the comparator code longhand ("if (a < b) return -1;" etc),
but I don't really think it's necessary.  bsearch is guaranteed to
correctly report that such a key is not present, even if it takes a
strange search path through the array due to inconsistent comparator
results.  So the test quoted above just serves to fail a bit more quickly,
but we certainly shouldn't be optimizing for the case of a bad ID.

> Overall, LGTM.

OK.  Will push shortly.

            regards, tom lane

On Thu, Sep 29, 2022 at 10:47:06AM -0400, Tom Lane wrote:
> OK.  Will push shortly.

Thanks!

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