Re: SYSTEM_USER reserved word implementation

Hi,

On 9/7/22 5:48 PM, Jacob Champion wrote:
> 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>?

Thanks for looking at it!
Good catch, V4 coming soon will make use of <link> instead.

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

Agree, will move it to the Session Information Functions in V4.

> 
>> +               /* 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()?

Fair point, V4 will make use of 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).

Will add additional check for safety in V4.


> 
> 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?

Good catch, will do in V4.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com