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

On Fri, Nov 01, 2024 at 02:47:38PM -0700, Jacob Champion wrote:
> On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> Could it be more transparent to use a "startup" or
>> "starting"" state instead that gets also used by pgstat_bestart() in
>> the case of the patch where !pre_auth?
>
> Done. (I've used "starting".)

0003 looks much cleaner this way.

> Added a new "Auth" class (to cover both authn and authz during
> startup), plus documentation.

+PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
+PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."

Is "local" required for both? Perhaps just use "the PAM service".

+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."

We don't document SID in doc/. So perhaps this should add be "SID
(system identifier)".

+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."

UPN is mentioned once in doc/ already. Perhaps this one is OK left
alone..

Except for these tweaks 0004 looks OK.

> The more I look at this, the more uneasy I feel about the goal. Best I
> can tell, the pre-auth step can't ignore irrelevant fields, because
> they may contain junk from the previous owner of the shared memory. So
> if we want to optimize, we can only change the second step to skip
> fields that were already filled in by the pre-auth step.
>
> That has its own problems: not every backend type uses the pre-auth
> step in the current patch. Which means a bunch of backends that don't
> benefit from the two-step initialization nevertheless have to either
> do two PGSTAT_BEGIN_WRITE_ACTIVITY() dances in a row, or else we
> duplicate a bunch of the logic to make sure they maintain the same
> efficient code path as before.
>
> Finally, if we're okay with all of that, future maintainers need to be
> careful about which fields get copied in the first (preauth) step, the
> second step, or both. GSS, for example, can be set up during transport
> negotiation (first step) or authentication (second step), so we have
> to duplicate the logic there. SSL is currently first-step-only, I
> think -- but are we sure we want to hardcode the assumption that cert
> auth can't change any of those parameters after the transport has been
> established? (I've been brainstorming ways we might use TLS 1.3's
> post-handshake CertificateRequest, for example.)

The future field maintenance and what one would need to think more
about in the future is a good point. I still feel slightly uneasy
about the way 0003 is shaped with its new pgstat_bestart_pre_auth(),
but I think that I'm just going to put my hands down on 0003 and see
if I can finish with something I'm a bit more comfortable with. Let's
see..

> So before I commit to this path, I just want to double-check that all
> of the above sounds good and non-controversial. :)

The goal of the thread is sound.

I'm OK with 0002 to add the wait parameter to BackgroundPsql and be
able to take some actions until a manual wait_connect(). I'll go do
this one. Also perhaps 0001 while on it but I am a bit puzzled by the
removal of the three ok() calls in 037_invalid_database.pl.
--
Michael