Re: System username in pg_stat_activity

On Wed, Jan 10, 2024 at 2:56 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Jan 10, 2024 at 02:08:03PM +0100, Magnus Hagander wrote:
> > On Wed, Jan 10, 2024 at 1:44 PM Aleksander Alekseev
> > <aleksander@timescale.com> wrote:
> > >
> > > Hi,
> > >
> > > Thanks for the patch.
>
> +1
>
> > > > This overlaps with for example the values in pg_stat_gss, but it will
> > > > include values for authentication methods that don't have their own
> > > > view such as peer/ident. gss/ssl info will of course still be shown,
> > > > it is just in more than one place.
>
> Yeah, I think that's a good idea.
>
> > > It hurts my sense of beauty that usename and authname are of different
> > > types. But if I'm the only one, maybe we can close our eyes on this.
> > > Also I suspect that placing usename and authname in a close proximity
> > > can be somewhat confusing. Perhaps adding authname as the last column
> > > of the view will solve both nitpicks?
> >
> > But it should probably actually be name, given that's the underlying
> > datatype. I kept changing it around and ended up half way in
> > between...
> >
> >
> > > ```
> > > +    /* Information about the authenticated user */
> > > +    char        st_authuser[NAMEDATALEN];
> > > ```
> > >
> > > Well, here it's called "authuser" and it looks like the intention was
> > > to use `name` datatype... I suggest using "authname" everywhere for
> > > consistency.
>
> I think it depends what we want the new field to reflect. If it is the exact
> same thing as the SYSTEM_USER then I think it has to be text (as the SYSTEM_USER
> is made of "auth_method:identity"). Now if we want it to be "only" the identity
> part of it, then the `name` datatype would be fine. I'd vote for the exact same
> thing as the SYSTEM_USER (means auth_method:identity).

I definitely think it should be the same. If it's not exactly the
same, then it should be *two* columns, one with auth method and one
with the name.

And thinking more about it maybe that's cleaner, because that makes it
easier to do things like filter based on auth method?


> > +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>authname</structfield> <type>name</type>
> > +      </para>
> > +      <para>
> > +       The authentication method and identity (if any) that the user
> > +       used to log in. It contains the same value as
> > +       <xref linkend="system-user" /> returns in the backend.
> > +      </para></entry>
> > +     </row>
>
> I'm fine with auth_method:identity.
>
> > +            S.authname,
>
> What about using system_user as the field name? (because if we keep
> auth_method:identity it's not really the authname anyway).

I was worried system_user or sysuser would both be confusing with the
fact that we have usesysid -- which would reference a *different*
sys...


> Also, what about adding a test in say 003_peer.pl to check that the value matches
> the SYSTEM_USER one?

Yeah, that's a good idea I think. I quickly looked over for tests and
couldn't really find any for pg_stat_activity, btu we can definitely
piggyback them in one or more of the auth tests.

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