Re: System username in pg_stat_activity

On Wed, Jan 10, 2024 at 1:44 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> Thanks for the patch.
>
> > The attached patch adds a column "authuser" to pg_stat_activity which
> > contains the username of the externally authenticated user, being the
> > same value as the SYSTEM_USER keyword returns in a backend.
>
> I believe what was meant is "authname", not "authuser".
>
> > 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.
> >
> > I was originally thinking this column should be "sysuser" to map to
> > the keyword, but since we already have "usesysid" as a column name in
> > pg_stat_activity I figured that could be confusing since it actually
> > means something completely different. But happy to change that back if
> > people think that's better.
>
> This part of the documentation is wrong:
>
> ```
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>authname</structfield> <type>name</type>
> +      </para>
> ```
>
> Actually the type is `text`:
>
> ```
> =# \d  pg_stat_activity ;
>                       View "pg_catalog.pg_stat_activity"
>       Column      |           Type           | Collation | Nullable | Default
> ------------------+--------------------------+-----------+----------+---------
>  datid            | oid                      |           |          |
>  datname          | name                     |           |          |
>  pid              | integer                  |           |          |
>  leader_pid       | integer                  |           |          |
>  usesysid         | oid                      |           |          |
>  usename          | name                     |           |          |
>  authname         | text                     |           |          |
> ```
>
> 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.

Yeah, I flipped back and forth a few times and clearly got stuck in
the middle. They should absolutely be the same everywhere - whatever
name is used it should be consistent.


> Since the patch affects pg_proc.dat I believe the commit message
> should remind bumping the catalog version.

Yes.

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