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

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