On Fri, Nov 01, 2024 at 02:47:38PM -0700, Jacob Champion wrote: > On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <mich...@paquier.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
signature.asc
Description: PGP signature
