Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
Andres Freund <andres@anarazel.de>
From: Andres Freund <andres@anarazel.de>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Michael Paquier <michael@paquier.xyz>, Robert Haas <robertmhaas@gmail.com>, Noah Misch <noah@leadboat.com>, Andrew Dunstan <andrew@dunslane.net>,
PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Euler Taveira <euler.taveira@enterprisedb.com>,
Daniel Gustafsson <daniel@yesql.se>
Date: 2025-03-13T17:56:42Z
Lists: pgsql-hackers
On 2025-03-13 10:29:49 -0700, Jacob Champion wrote: > On Thu, Mar 13, 2025 at 9:56 AM Andres Freund <andres@anarazel.de> wrote: > > I am wondering if PAM is so fundamentally incompatible with handling > > interrupts / a non-blocking interface that we have little choice but to > > eventually remove it... > > Given the choice between a usually-working PAM module with known > architectural flaws, and not having PAM at all, I think many users > would rather continue using what's working for them. authentication_timeout currently doesn't reliably work while in some auth methods, nor does pg_terminate_backend() etc. That's IMO is rather bad from a DOSability perspective. The fact that some auth methods are broken like that has had a sizable negative impact on postgres for a long time. Not just when those methods are used, but also architecturally. It's e.g. one of the main reasons we need the ugly escalating logic in postmaster shutdowns to send SIGQUITs and then SIGKILL after a while, because we don't have a reliable way of terminating backends normally. This used to be way worse because historically postgres considered it sane (why, I have no idea) to ereport() in timeout functions, which then occasionally lead to backends stuck in malloc locks etc. > > FWIW, I continue to think that it's better to invest in making more auth > > methods non-blocking, rather than adding wait events for code that could maybe > > sometimes wait on different things internally. > > I think we disagree on the either/or nature of that. If I can get > proof that a certain thing is causing bugs in the wild, then I have > ammunition to fix that thing. FWIW, I've have repeatedly seen production issues due to authentication timeout not working for some auth methods. It's not hard to see why - e.g. a non-resonsive radius server just leaves the backend hanging in select(). Even though it would get interrupted by signals, we'll just retry without even checking interrupts / timeouts :(. > Right now there is no visibility, and my interest in rewriting old > authentication methods without bug reports to motivate that work is pretty > low. I'm not willing to sign up for that at the moment. Fair enough. > (But I do really appreciate the review. I'm just feeling crispy about > the overall result...) Also fair enough :) Greetings, Andres Freund
Commits
-
Fix race condition in TAP test 007_pre_auth
- e2080261cc8c 18.0 landed
-
Split pgstat_bestart() into three different routines
- c76db55c9085 18.0 landed
-
backport: Extend background_psql() to be able to start asynchronously
- 6af51bf05a6a 13.21 landed
- 7c07ab62aeb7 14.18 landed
- c0bc11aebb01 15.13 landed
- 6ab58d506bba 16.9 landed
- 49b6f4a02b23 17.5 landed
-
backport: Improve handling of empty query results in BackgroundPsql
- 3c562b58c20e 13.21 landed
- 3170aece14b8 14.18 landed
- f4b08ccb4ec8 15.13 landed
- fbfd38662f72 16.9 landed
- 31a242e90c88 17.5 landed
-
Improve handling of empty query results in BackgroundPsql::query()
- 70291a3c66ec 18.0 landed
-
Extend Cluster.pm's background_psql() to be able to start asynchronously
- ba08edb06545 18.0 landed
-
dblink: Replace WAIT_EVENT_EXTENSION with custom wait events
- c789f0f6cc5d 17.0 cited