Re: System username in pg_stat_activity

On Thu, Jan 11, 2024 at 5:55 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Jan 11, 2024 at 02:24:58PM +0100, Magnus Hagander wrote:
> > On Wed, Jan 10, 2024 at 3:12 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > > If we go the 2 fields way, then what about auth_identity and auth_method then?
> >
> >
> > Here is an updated patch based on this idea.
>
> Thanks!
>
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>auth_method</structfield> <type>text</type>
> + </para>
> + <para>
> + The authentication method used for authenticating the connection, or
> + NULL for background processes.
> + </para></entry>
>
> 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.

> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>auth_identity</structfield> <type>text</type>
> + </para>
> + <para>
> + The identity (if any) that the user presented during the authentication
> + cycle before they were assigned a database role. Contains the same
> + value as <xref linkend="system-user" />
>
> 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.

> - what about "Contains the same value as the identity part in <xref linkend="system-user" />"?
>
> + /*
> + * 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.

> +# pg_stat_activity shold contain trust and empty string for trust auth
>
> typo: s/shold/should/

Ops.

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

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/