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

From: Jacob Champion <[email-masked]>
Date: Tue, 3 Sep 2024 14:47:57 -0700

On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <[email-masked]> wrote:
> On Fri, Aug 30, 2024 at 04:10:32PM -0400, Andrew Dunstan wrote:
> > Patch 0001 looks sane to me.
> So does 0002 to me.

Thanks both!

> I'm not much a fan of the addition of
> pgstat_bestart_pre_auth(), which is just a shortcut to set a different
> state in the backend entry to tell that it is authenticating.  Is
> authenticating the term for this state of the process startups,
> actually?  Could it be more transparent to use a "startup" or
> "starting"" state instead

Yeah, I think I should rename that. Especially if we adopt new wait
states to make it obvious where we're stuck.

"startup", "starting", "initializing", "connecting"...?

> 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?

> 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?
2) Is the level at which I've inserted calls to
pgstat_report_wait_start()/_end() sane and maintainable?

> and these can be seen in pg_stat_activity once we get out of
> PGSTAT_END_WRITE_ACTIVITY() (err.. Right?).

It doesn't look like pgstat_report_wait_start() uses that machinery.

--Jacob

---

From: Michael Paquier <[email-masked]>
Date: Tue, 10 Sep 2024 14:29:57 +0900

> 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.

> 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"?

> 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

---

From: Jacob Champion <[email-masked]>
Date: Wed, 11 Sep 2024 14:29:49 -0700

> setup for backend entries depending on the authentication path taken.

I can try to separate them out. I'm a little wary of messing with the
CRITICAL_SECTION guarantees, though. I thought the idea was that you
filled in the entire struct to prevent tearing. (If I've misunderstood
that, please let me know :D)

> Perhaps just use a new

Sounds good.

> Couldn't it be better to have a one-one mapping

(I have no strong opinions on this myself, but while the debate is
ongoing, I'll work on a version of the patch with more detailed wait
events. It's easy to collapse them again if that gets the most votes.)

> are you planning for more?

I can test for specific contents of the entry, if you'd like. My
primary goal was to test that an entry shows up if that part of the
code hangs. I think a regression would otherwise go completely
unnoticed.

Thanks!
--Jacob

---

From: Michael Paquier <[email-masked]>
Date: Thu, 12 Sep 2024 08:42:25 +0900

> that, please let me know :D)

Hm, yeah.  We surely should be careful about the consequences of that.
Setting up twice the structure as the patch proposes is kind of
a weird concept, but it feels to me that we should split that and set
the fields in the pre-auth step and ignore the irrelevant ones, then
complete the rest in a second step.  We are going to do that anyway if
we want to be able to have backend entries earlier in the
authentication phase.

> events. It's easy to collapse them again if that gets the most votes.)

Thanks.  Robert is arguing upthread about more granularity, which is
also what I understand is the original intention of the wait events.
Noah has a different view.  Let's see where it goes but I've given my
opinion.

> code hangs. I think a regression would otherwise go completely

Perhaps that would be useful, not sure.  Based on my first
impressions, I'd tend to say no to these extra test cycles, but I'm
okay to be proved wrong, as well.
--
Michael