Horton

Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

Noah Misch <noah@leadboat.com>

View thread View on postgr.es
From: Noah Misch <noah@leadboat.com>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Euler Taveira <euler.taveira@enterprisedb.com>
Date: 2024-06-30T17:48:12Z
Lists: pgsql-hackers 
On Mon, May 06, 2024 at 02:23:38PM -0700, Jacob Champion wrote:
>     =# select * from pg_stat_activity where state = 'authenticating';
>     -[ RECORD 1 ]----+------------------------------
>     datid            |
>     datname          |
>     pid              | 745662
>     leader_pid       |
>     usesysid         |
>     usename          |
>     application_name |
>     client_addr      | 127.0.0.1
>     client_hostname  |
>     client_port      | 38304
>     backend_start    | 2024-05-06 11:25:23.905923-07
>     xact_start       |
>     query_start      |
>     state_change     |
>     wait_event_type  | Client
>     wait_event       | ClientRead
>     state            | authenticating
>     backend_xid      |
>     backend_xmin     | 784
>     query_id         |
>     query            |
>     backend_type     | client backend

That looks like a reasonable user experience.  Is any field newly-nullable?

> = Open Issues, Idle Thoughts =
> 
> Maybe it's wishful thinking, but it'd be cool if a misbehaving
> authentication exchange did not impact replicas in any way. Is there a
> way to make that opening transaction lighterweight?

You could release the xmin before calling PAM or LDAP.  If you've copied all
relevant catalog content to local memory, that's fine to do.  That said, it
may be more fruitful to arrange for authentication timeout to cut through PAM
etc.  Hanging connection slots hurt even if they lack an xmin.  I assume it
takes an immediate shutdown to fix them?

> Would anyone like me to be more aggressive, and create a pgstat entry
> as soon as we have the opening transaction? Or... as soon as a
> connection is made?

All else being equal, I'd like backends to have one before taking any lmgr
lock or snapshot.

> I haven't decided how to test these patches. Seems like a potential
> use case for injection points, but I think I'd need to preload an
> injection library rather than using the existing extension. Does that
> seem like an okay way to go?

Yes.

Thanks,
nm

Commits

  1. Fix race condition in TAP test 007_pre_auth

  2. Split pgstat_bestart() into three different routines

  3. backport: Extend background_psql() to be able to start asynchronously

  4. backport: Improve handling of empty query results in BackgroundPsql

  5. Improve handling of empty query results in BackgroundPsql::query()

  6. Extend Cluster.pm's background_psql() to be able to start asynchronously

  7. dblink: Replace WAIT_EVENT_EXTENSION with custom wait events