Hi, On Fri, Jan 12, 2024 at 05:16:53PM +0100, Magnus Hagander wrote: > On Thu, Jan 11, 2024 at 5:55 PM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > > I'm wondering if it would make sense to populate it for parallel workers > > too. > > I think it's doable thanks to d951052, but I'm not sure it's worth it (one > > could > > join based on the leader_pid though). OTOH that would be consistent with > > how the SYSTEM_USER behaves with parallel workers (it's populated). > > I guess one could conceptually argue that "authentication happens int > he leader". But we do populate it with the other user records, and > it'd be weird if this one was excluded. > > The tricky thing is that pgstat_bestart() is called long before we > deserialize the data. But from what I can tell it should be safe to > change it per the attached? That should be AFAICT an extremely short > window of time longer before we report it, not enough to matter.
Thanks! Yeah, that seems reasonable to me. Also, I think we should remove the "MyProcPort" test here then (looking at v3): + if (MyProcPort && MyClientConnectionInfo.authn_id) + strlcpy(lbeentry.st_auth_identity, MyClientConnectionInfo.authn_id, NAMEDATALEN); + else + MemSet(&lbeentry.st_auth_identity, 0, sizeof(lbeentry.st_auth_identity)); to get the st_auth_identity propagated to the parallel workers. > > > > Same remark regarding the parallel workers case +: > > > > - Would it be better to use the `name` datatype for auth_identity? > > I've been going back and forth. And I think my conclusion is that it's > not a postgres identifier, so it shouldn't be. See the earlier > discussion, and for example that that's what we do for cert names when > SSL is used. Yeah, Okay let's keep text then. > > > - what about "Contains the same value as the identity part in <xref > > linkend="system-user" />"? Not sure, but looks like you missed this comment? > > > > + /* > > + * Trust doesn't set_authn_id(), but we still need > > to store the > > + * auth_method > > + */ > > + MyClientConnectionInfo.auth_method = uaTrust; > > > > +1, I think it is useful here to provide "trust" and not a NULL value in the > > context of this patch. > > Yeah, that's probably "independently correct", but actually useful here. +1 > > +# Users with md5 auth should show both auth method and name in > > pg_stat_activity > > > > what about "show both auth method and identity"? > > Good spot, yeah, I changed it over to identity everywhere else so it > should be here as well. Did you forget to share the new revision (aka v4)? I can only see the "reorder_parallel_worker_bestart.patch" attached. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
