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

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(at)paquier(dot)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