On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier <michael@paquier.xyz> wrote:
> 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.
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
> "Authentication" class, as in "The server is waiting for an
> authentication operation to complete"?
Sounds good.
> Couldn't it be better to have a one-one mapping
> instead, adding twelve entries in wait_event_names.txt?
(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.)
> 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?
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
