Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
Jacob Champion <jacob.champion@enterprisedb.com>
From: Jacob Champion <jacob.champion@enterprisedb.com>
To: Andres Freund <andres@anarazel.de>
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>
Date: 2024-11-07T18:44:25Z
Lists: pgsql-hackers
On Thu, Nov 7, 2024 at 10:12 AM Andres Freund <andres@anarazel.de> wrote: > I don't understand why the pgstat_bestart()/pgstat_bestart_pre_auth() split > makes sense. The latter is going to redo most of the work that the former > did. What's the point of that? > > Why not have a new function that initializes just the missing additional > information? Or for that matter, why not move most of what pgstat_bestart() > does into pgstat_beinit()? I talk about that up above [1]. I agree that this is all complicated and fragile, but at the moment, I think splitting things apart is not going to reduce the complexity in any way. I'm all ears for a different approach, though (and it sounds like Michael is taking a stab at it too). > This doesn't really seem like it's actually using wait events to describe > waits. The new wait events cover stuff like memory allocations etc, see > e.g. pg_SSPI_make_upn(). I've also asked about the "scope" of the waits in the OP [2]. I can move them downwards in the stack, if you'd prefer. All of these are intended to cover parts of the code that can actually hang, but for things like SSPI I'm just working off of inspection and Win32 documentation. So if it's not actually true that some of these call points can hang, let me know and I can remove them. (For the particular example you called out, I'm just trying to cover both calls to TranslateName() in a maintainable place. The documentation says "TranslateName fails if it cannot bind to Active Directory on a domain controller." which seemed pretty wait-worthy to me.) > This isn't just pedantry - all the relevant code really needs to be rewritten > to allow the blocking to happen in an interruptible way, otherwise > authentication timeout etc can't realiably work. Once that's done you can > actually define useful wait events too. I agree that would be amazing! I'm not about to tackle reliable interrupts for all of the current blocking auth code for v18, though. I'm just trying to make it observable when we do something that blocks. --Jacob [1] https://www.postgresql.org/message-id/CAOYmi%2BkLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
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