Re: SYSTEM_USER reserved word implementation

On 9/7/22 07:46, Drouvot, Bertrand wrote:
> Except the Nit above, that looks all good to me.

A few additional comments:

> +        assigned a database role. It is represented as
> +        <literal>auth_method:identity</literal> or
> +        <literal>NULL</literal> if the user has not been authenticated (for
> +        example if <xref linkend="auth-trust"/> has been used).
> +       </para></entry>

This is rendered as

    ... (for example if Section 21.4 has been used).

which IMO isn't too helpful. Maybe a <link> would read better than an
<xref>?

Also, this function's placement in the docs (with the System Catalog
Information Functions) seems wrong to me. I think it should go up above
in the Session Information Functions, with current_user et al.

> +               /* Build system user as auth_method:authn_id */
> +               char       *system_user;
> +               Size            authname_len = strlen(auth_method);
> +               Size            authn_id_len = strlen(authn_id);
> +
> +               system_user = palloc0(authname_len + authn_id_len + 2);
> +               strcat(system_user, auth_method);
> +               strcat(system_user, ":");
> +               strcat(system_user, authn_id);

If we're palloc'ing anyway, can this be replaced with a single psprintf()?

> +       /* Initialize SystemUser now that MyClientConnectionInfo is restored. */
> +       InitializeSystemUser(MyClientConnectionInfo.authn_id,
> +                                                hba_authname(MyClientConnectionInfo.auth_method));

It makes me a little nervous to call hba_authname(auth_method) without
checking to see that auth_method is actually valid (which is only true
if authn_id is not NULL).

We could pass the bare auth_method index, or update the documentation
for auth_method to state that it's guaranteed to be zero if authn_id is
NULL (and then enforce that).

>         case SVFOP_CURRENT_USER:
>         case SVFOP_USER:
>         case SVFOP_SESSION_USER:
> +       case SVFOP_SYSTEM_USER:
>         case SVFOP_CURRENT_CATALOG:
>         case SVFOP_CURRENT_SCHEMA:
>             svf->type = NAMEOID;

Should this be moved to use TEXTOID instead?

Thanks,
--Jacob