Horton

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

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Andrew Dunstan <andrew@dunslane.net>, Noah Misch <noah@leadboat.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Euler Taveira <euler.taveira@enterprisedb.com>
Date: 2024-09-10T05:29:57Z
Lists: pgsql-hackers 
On Tue, Sep 03, 2024 at 02:47:57PM -0700, Jacob Champion wrote:
> On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <michael@paquier.xyz> wrote:
>> that gets also used by pgstat_bestart() in
>> the case of the patch where !pre_auth?
> 
> To clarify, do you want me to just add the new boolean directly to
> pgstat_bestart()'s parameter list?

No.  My question was about splitting pgstat_bestart() and
pgstat_bestart_pre_auth() in a cleaner way, because authenticated
connections finish by calling both, meaning that we do twice the same
setup for backend entries depending on the authentication path taken.
That seems like a waste.

>> The addition of the new wait event states in 0004 is a good idea,
>> indeed,
> 
> Thanks! Any thoughts on the two open questions for it?:
> 1) Should we add a new wait event class rather than reusing IPC?

A new category would be more adapted.  IPC is not adapted because are
not waiting for another server process.  Perhaps just use a new
"Authentication" class, as in "The server is waiting for an
authentication operation to complete"?

> 2) Is the level at which I've inserted calls to
> pgstat_report_wait_start()/_end() sane and maintainable?

These don't worry me.  You are adding twelve event points with only 5
new wait names.  Couldn't it be better to have a one-one mapping
instead, adding twelve entries in wait_event_names.txt?

I am not really on board with the test based on injection points
proposed, though.  It checks that the "authenticating" flag is set in
pg_stat_activity, but it does nothing else.  That seems limited.  Or
are you planning for more?
--
Michael

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