On Fri, Feb 28, 2025 at 12:40:13PM -0800, Jacob Champion wrote: > v9 removes the first call, and moves the second (now only) call up and > out of the if/else chain, just past client authentication. The SSL > pre-auth tests have been removed.
I have put my eyes on 0001, and this version looks sensible here, just
tweaked a bit the comments after a closer lookup and adjusted a few
things, nothing huge..
/* Update app name to current GUC setting */
+ /* TODO: ask the list: maybe do this before setting STATE_UNDEFINED? */
if (application_name)
pgstat_report_appname(application_name);
This has always been set last and it's still the case in the patch, so
let's just remove that.
--
Michael
From 301c381261c08efdcf2b129b8150db98eb101a97 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champ...@enterprisedb.com>
Date: Fri, 3 May 2024 15:54:58 -0700
Subject: [PATCH v10] pgstat: report in earlier with STATE_STARTING
Split pgstat_bestart() into three phases for better observability:
1) pgstat_bestart_initial() reports a 'starting' state while waiting for
backend initialization and client authentication to complete. Since
we hold a transaction open for a good amount of that, and some
authentication methods call out to external systems, having an early
pg_stat_activity entry helps DBAs debug when things go badly wrong.
2) pgstat_bestart_security() reports the SSL/GSS status of the
connection. Some backends don't call this at all; others call it
after client authentication.
3) pgstat_bestart_final() reports the user and database IDs, takes the
entry out of STATE_STARTING, and reports the application_name.
TODO: should the order of those last two be swapped?
---
src/include/utils/backend_status.h | 5 +-
src/backend/postmaster/auxprocess.c | 3 +-
src/backend/utils/activity/backend_status.c | 215 +++++++++++++-------
src/backend/utils/adt/pgstatfuncs.c | 3 +
src/backend/utils/init/postinit.c | 40 +++-
src/test/authentication/Makefile | 4 +
src/test/authentication/meson.build | 4 +
src/test/authentication/t/007_pre_auth.pl | 81 ++++++++
doc/src/sgml/monitoring.sgml | 6 +
9 files changed, 275 insertions(+), 86 deletions(-)
create mode 100644 src/test/authentication/t/007_pre_auth.pl
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index d3d4ff6c5c9a..1c9b4fe14d06 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -24,6 +24,7 @@
typedef enum BackendState
{
STATE_UNDEFINED,
+ STATE_STARTING,
STATE_IDLE,
STATE_RUNNING,
STATE_IDLEINTRANSACTION,
@@ -309,7 +310,9 @@ extern void BackendStatusShmemInit(void);
/* Initialization functions */
extern void pgstat_beinit(void);
-extern void pgstat_bestart(void);
+extern void pgstat_bestart_initial(void);
+extern void pgstat_bestart_security(void);
+extern void pgstat_bestart_final(void);
extern void pgstat_clear_backend_activity_snapshot(void);
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index ff366ceb0fc7..4f6795f72650 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -78,7 +78,8 @@ AuxiliaryProcessMainCommon(void)
/* Initialize backend status information */
pgstat_beinit();
- pgstat_bestart();
+ pgstat_bestart_initial();
+ pgstat_bestart_final();
/* register a before-shutdown callback for LWLock cleanup */
before_shmem_exit(ShutdownAuxiliaryProcess, 0);
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 5f68ef26adc6..1a4ca2b179ca 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -253,31 +253,23 @@ pgstat_beinit(void)
on_shmem_exit(pgstat_beshutdown_hook, 0);
}
-
-/* ----------
- * pgstat_bestart() -
+/* ---------
+ * pgstat_bestart_initial() -
*
- * Initialize this backend's entry in the PgBackendStatus array.
- * Called from InitPostgres.
+ * Initialize this backend's entry in the PgBackendStatus array.
+ * Called from InitPostgres() and AuxiliaryProcessMainCommon().
*
- * Apart from auxiliary processes, MyDatabaseId, session userid, and
- * application_name must already be set (hence, this cannot be combined
- * with pgstat_beinit). Note also that we must be inside a transaction
- * if this isn't an aux process, as we may need to do encoding conversion
- * on some strings.
- *----------
+ * Clears out a new pgstat entry, initializing it to suitable defaults and
+ * reporting STATE_STARTING. Backends should continue filling in any
+ * transport security details as needed with pgstat_bestart_security(), and
+ * must finally exit STATE_STARTING by calling pgstat_bestart_final().
+ * ---------
*/
void
-pgstat_bestart(void)
+pgstat_bestart_initial(void)
{
volatile PgBackendStatus *vbeentry = MyBEEntry;
PgBackendStatus lbeentry;
-#ifdef USE_SSL
- PgBackendSSLStatus lsslstatus;
-#endif
-#ifdef ENABLE_GSS
- PgBackendGSSStatus lgssstatus;
-#endif
/* pgstats state must be initialized from pgstat_beinit() */
Assert(vbeentry != NULL);
@@ -297,14 +289,6 @@ pgstat_bestart(void)
unvolatize(PgBackendStatus *, vbeentry),
sizeof(PgBackendStatus));
- /* These structs can just start from zeroes each time, though */
-#ifdef USE_SSL
- memset(&lsslstatus, 0, sizeof(lsslstatus));
-#endif
-#ifdef ENABLE_GSS
- memset(&lgssstatus, 0, sizeof(lgssstatus));
-#endif
-
/*
* Now fill in all the fields of lbeentry, except for strings that are
* out-of-line data. Those have to be handled separately, below.
@@ -315,15 +299,8 @@ pgstat_bestart(void)
lbeentry.st_activity_start_timestamp = 0;
lbeentry.st_state_start_timestamp = 0;
lbeentry.st_xact_start_timestamp = 0;
- lbeentry.st_databaseid = MyDatabaseId;
-
- /* We have userid for client-backends, wal-sender and bgworker processes */
- if (lbeentry.st_backendType == B_BACKEND
- || lbeentry.st_backendType == B_WAL_SENDER
- || lbeentry.st_backendType == B_BG_WORKER)
- lbeentry.st_userid = GetSessionUserId();
- else
- lbeentry.st_userid = InvalidOid;
+ lbeentry.st_databaseid = InvalidOid;
+ lbeentry.st_userid = InvalidOid;
/*
* We may not have a MyProcPort (eg, if this is the autovacuum process).
@@ -336,46 +313,10 @@ pgstat_bestart(void)
else
MemSet(&lbeentry.st_clientaddr, 0, sizeof(lbeentry.st_clientaddr));
-#ifdef USE_SSL
- if (MyProcPort && MyProcPort->ssl_in_use)
- {
- lbeentry.st_ssl = true;
- lsslstatus.ssl_bits = be_tls_get_cipher_bits(MyProcPort);
- strlcpy(lsslstatus.ssl_version, be_tls_get_version(MyProcPort), NAMEDATALEN);
- strlcpy(lsslstatus.ssl_cipher, be_tls_get_cipher(MyProcPort), NAMEDATALEN);
- be_tls_get_peer_subject_name(MyProcPort, lsslstatus.ssl_client_dn, NAMEDATALEN);
- be_tls_get_peer_serial(MyProcPort, lsslstatus.ssl_client_serial, NAMEDATALEN);
- be_tls_get_peer_issuer_name(MyProcPort, lsslstatus.ssl_issuer_dn, NAMEDATALEN);
- }
- else
- {
- lbeentry.st_ssl = false;
- }
-#else
lbeentry.st_ssl = false;
-#endif
-
-#ifdef ENABLE_GSS
- if (MyProcPort && MyProcPort->gss != NULL)
- {
- const char *princ = be_gssapi_get_princ(MyProcPort);
-
- lbeentry.st_gss = true;
- lgssstatus.gss_auth = be_gssapi_get_auth(MyProcPort);
- lgssstatus.gss_enc = be_gssapi_get_enc(MyProcPort);
- lgssstatus.gss_delegation = be_gssapi_get_delegation(MyProcPort);
- if (princ)
- strlcpy(lgssstatus.gss_princ, princ, NAMEDATALEN);
- }
- else
- {
- lbeentry.st_gss = false;
- }
-#else
lbeentry.st_gss = false;
-#endif
- lbeentry.st_state = STATE_UNDEFINED;
+ lbeentry.st_state = STATE_STARTING;
lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
lbeentry.st_progress_command_target = InvalidOid;
lbeentry.st_query_id = UINT64CONST(0);
@@ -417,20 +358,146 @@ pgstat_bestart(void)
lbeentry.st_clienthostname[NAMEDATALEN - 1] = '\0';
lbeentry.st_activity_raw[pgstat_track_activity_query_size - 1] = '\0';
+ /* These structs can just start from zeroes each time */
#ifdef USE_SSL
- memcpy(lbeentry.st_sslstatus, &lsslstatus, sizeof(PgBackendSSLStatus));
+ memset(lbeentry.st_sslstatus, 0, sizeof(PgBackendSSLStatus));
#endif
#ifdef ENABLE_GSS
- memcpy(lbeentry.st_gssstatus, &lgssstatus, sizeof(PgBackendGSSStatus));
+ memset(lbeentry.st_gssstatus, 0, sizeof(PgBackendGSSStatus));
#endif
PGSTAT_END_WRITE_ACTIVITY(vbeentry);
+}
+
+/* --------
+ * pgstat_bestart_security() -
+ *
+ * Fill in SSL and GSS information for the pgstat entry. This is the second
+ * optional step taken when filling a backend's entry, not required for
+ * auxiliary processes.
+ *
+ * This should only be called from backends with a MyProcPort.
+ * --------
+ */
+void
+pgstat_bestart_security(void)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+ bool ssl = false;
+ bool gss = false;
+#ifdef USE_SSL
+ PgBackendSSLStatus lsslstatus;
+ PgBackendSSLStatus *st_sslstatus;
+#endif
+#ifdef ENABLE_GSS
+ PgBackendGSSStatus lgssstatus;
+ PgBackendGSSStatus *st_gssstatus;
+#endif
+
+ /* pgstats state must be initialized from pgstat_beinit() */
+ Assert(beentry != NULL);
+ Assert(MyProcPort); /* otherwise there's no point */
+
+#ifdef USE_SSL
+ st_sslstatus = beentry->st_sslstatus;
+ memset(&lsslstatus, 0, sizeof(lsslstatus));
+
+ if (MyProcPort->ssl_in_use)
+ {
+ ssl = true;
+ lsslstatus.ssl_bits = be_tls_get_cipher_bits(MyProcPort);
+ strlcpy(lsslstatus.ssl_version, be_tls_get_version(MyProcPort), NAMEDATALEN);
+ strlcpy(lsslstatus.ssl_cipher, be_tls_get_cipher(MyProcPort), NAMEDATALEN);
+ be_tls_get_peer_subject_name(MyProcPort, lsslstatus.ssl_client_dn, NAMEDATALEN);
+ be_tls_get_peer_serial(MyProcPort, lsslstatus.ssl_client_serial, NAMEDATALEN);
+ be_tls_get_peer_issuer_name(MyProcPort, lsslstatus.ssl_issuer_dn, NAMEDATALEN);
+ }
+#endif
+
+#ifdef ENABLE_GSS
+ st_gssstatus = beentry->st_gssstatus;
+ memset(&lgssstatus, 0, sizeof(lgssstatus));
+
+ if (MyProcPort->gss != NULL)
+ {
+ const char *princ = be_gssapi_get_princ(MyProcPort);
+
+ gss = true;
+ lgssstatus.gss_auth = be_gssapi_get_auth(MyProcPort);
+ lgssstatus.gss_enc = be_gssapi_get_enc(MyProcPort);
+ lgssstatus.gss_delegation = be_gssapi_get_delegation(MyProcPort);
+ if (princ)
+ strlcpy(lgssstatus.gss_princ, princ, NAMEDATALEN);
+ }
+#endif
+
+ /*
+ * Update my status entry, following the protocol of bumping
+ * st_changecount before and after. We use a volatile pointer here to
+ * ensure the compiler doesn't try to get cute.
+ */
+ PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+
+ beentry->st_ssl = ssl;
+ beentry->st_gss = gss;
+
+#ifdef USE_SSL
+ memcpy(st_sslstatus, &lsslstatus, sizeof(PgBackendSSLStatus));
+#endif
+#ifdef ENABLE_GSS
+ memcpy(st_gssstatus, &lgssstatus, sizeof(PgBackendGSSStatus));
+#endif
+
+ PGSTAT_END_WRITE_ACTIVITY(beentry);
+}
+
+/* --------
+ * pgstat_bestart_final() -
+ *
+ * Finalizes the state of this backend's entry entry by filling in the
+ * user and database IDs, clearing STATE_STARTING, and reporting the
+ * application_name.
+ *
+ * We must be inside a transaction if this is not an auxiliary process, as
+ * we may need to do encoding conversion.
+ * --------
+ */
+void
+pgstat_bestart_final(void)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+ Oid userid;
+
+ /* pgstats state must be initialized from pgstat_beinit() */
+ Assert(beentry != NULL);
+
+ /* We have userid for client-backends, wal-sender and bgworker processes */
+ if (MyBackendType == B_BACKEND
+ || MyBackendType == B_WAL_SENDER
+ || MyBackendType == B_BG_WORKER)
+ userid = GetSessionUserId();
+ else
+ userid = InvalidOid;
+
+ /*
+ * Update my status entry, following the protocol of bumping
+ * st_changecount before and after. We use a volatile pointer here to
+ * ensure the compiler doesn't try to get cute.
+ */
+ PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+
+ beentry->st_databaseid = MyDatabaseId;
+ beentry->st_userid = userid;
+ beentry->st_state = STATE_UNDEFINED;
+
+ PGSTAT_END_WRITE_ACTIVITY(beentry);
/* Create the backend statistics entry */
if (pgstat_tracks_backend_bktype(MyBackendType))
pgstat_create_backend(MyProcNumber);
/* Update app name to current GUC setting */
+ /* TODO: ask the list: maybe do this before setting STATE_UNDEFINED? */
if (application_name)
pgstat_report_appname(application_name);
}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 0212d8d5906b..9172e1cb9d23 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -393,6 +393,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
switch (beentry->st_state)
{
+ case STATE_STARTING:
+ values[4] = CStringGetTextDatum("starting");
+ break;
case STATE_IDLE:
values[4] = CStringGetTextDatum("idle");
break;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 318600d6d02e..b428a59bdd26 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -59,6 +59,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
#include "utils/portal.h"
@@ -718,6 +719,20 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
InitProcessPhase2();
+ /* Initialize status reporting */
+ pgstat_beinit();
+
+ /*
+ * And initialize an entry in the PgBackendStatus array. That way, if
+ * LWLocks or third-party authentication should happen to hang, it is
+ * possible to retrieve some information about what is going on.
+ */
+ if (!bootstrap)
+ {
+ pgstat_bestart_initial();
+ INJECTION_POINT("init-pre-auth");
+ }
+
/*
* Initialize my entry in the shared-invalidation manager's array of
* per-backend data.
@@ -786,9 +801,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* Initialize portal manager */
EnablePortalManager();
- /* Initialize status reporting */
- pgstat_beinit();
-
/*
* Load relcache entries for the shared system catalogs. This must create
* at least entries for pg_database and catalogs used for authentication.
@@ -809,8 +821,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* The autovacuum launcher is done here */
if (AmAutoVacuumLauncherProcess())
{
- /* report this backend in the PgBackendStatus array */
- pgstat_bestart();
+ /* fill in the remainder of this entry in the PgBackendStatus array */
+ pgstat_bestart_final();
return;
}
@@ -884,6 +896,14 @@ InitPostgres(const char *in_dbname, Oid dboid,
am_superuser = superuser();
}
+ /* Report any SSL/GSS details for the session. */
+ if (MyProcPort != NULL)
+ {
+ Assert(!bootstrap);
+
+ pgstat_bestart_security();
+ }
+
/*
* Binary upgrades only allowed super-user connections
*/
@@ -953,8 +973,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* initialize client encoding */
InitializeClientEncoding();
- /* report this backend in the PgBackendStatus array */
- pgstat_bestart();
+ /* fill in the remainder of this entry in the PgBackendStatus array */
+ pgstat_bestart_final();
/* close the transaction we started above */
CommitTransactionCommand();
@@ -997,7 +1017,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
if (!bootstrap)
{
- pgstat_bestart();
+ pgstat_bestart_final();
CommitTransactionCommand();
}
return;
@@ -1197,9 +1217,9 @@ InitPostgres(const char *in_dbname, Oid dboid,
if ((flags & INIT_PG_LOAD_SESSION_LIBS) != 0)
process_session_preload_libraries();
- /* report this backend in the PgBackendStatus array */
+ /* fill in the remainder of this entry in the PgBackendStatus array */
if (!bootstrap)
- pgstat_bestart();
+ pgstat_bestart_final();
/* close the transaction we started above */
if (!bootstrap)
diff --git a/src/test/authentication/Makefile b/src/test/authentication/Makefile
index c4022dc829b6..8b5beced0806 100644
--- a/src/test/authentication/Makefile
+++ b/src/test/authentication/Makefile
@@ -13,6 +13,10 @@ subdir = src/test/authentication
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+EXTRA_INSTALL = src/test/modules/injection_points
+
+export enable_injection_points
+
check:
$(prove_check)
diff --git a/src/test/authentication/meson.build b/src/test/authentication/meson.build
index f6e48411c116..800b3a5ff40f 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -5,6 +5,9 @@ tests += {
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
'tap': {
+ 'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+ },
'tests': [
't/001_password.pl',
't/002_saslprep.pl',
@@ -12,6 +15,7 @@ tests += {
't/004_file_inclusion.pl',
't/005_sspi.pl',
't/006_login_trigger.pl',
+ 't/007_pre_auth.pl',
],
},
}
diff --git a/src/test/authentication/t/007_pre_auth.pl b/src/test/authentication/t/007_pre_auth.pl
new file mode 100644
index 000000000000..a638226dbaf1
--- /dev/null
+++ b/src/test/authentication/t/007_pre_auth.pl
@@ -0,0 +1,81 @@
+
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+# Tests for connection behavior prior to authentication.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q[
+log_connections = on
+]);
+
+$node->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# Connect to the server and inject a waitpoint.
+my $psql = $node->background_psql('postgres');
+$psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
+
+# From this point on, all new connections will hang during startup, just before
+# authentication. Use the $psql connection handle for server interaction.
+my $conn = $node->background_psql('postgres', wait => 0);
+
+# Wait for the connection to show up.
+my $pid;
+while (1)
+{
+ $pid = $psql->query(
+ "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
+ last if $pid ne "";
+
+ usleep(100_000);
+}
+
+note "backend $pid is authenticating";
+ok(1, 'authenticating connections are recorded in pg_stat_activity');
+
+# Detach the waitpoint and wait for the connection to complete.
+$psql->query_safe("SELECT injection_points_wakeup('init-pre-auth');");
+$conn->wait_connect();
+
+# Make sure the pgstat entry is updated eventually.
+while (1)
+{
+ my $state =
+ $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
+ last if $state eq "idle";
+
+ note "state for backend $pid is '$state'; waiting for 'idle'...";
+ usleep(100_000);
+}
+
+ok(1, 'authenticated connections reach idle state in pg_stat_activity');
+
+$psql->query_safe("SELECT injection_points_detach('init-pre-auth');");
+$psql->quit();
+$conn->quit();
+
+done_testing();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 9178f1d34efd..16646f560e8d 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -899,6 +899,12 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
Current overall state of this backend.
Possible values are:
<itemizedlist>
+ <listitem>
+ <para>
+ <literal>starting</literal>: The backend is in initial startup. Client
+ authentication is performed during this phase.
+ </para>
+ </listitem>
<listitem>
<para>
<literal>active</literal>: The backend is executing a query.
--
2.47.2
signature.asc
Description: PGP signature
