From: | "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Jacob Champion <jchampion(at)timescale(dot)com>, "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Joe Conway <mail(at)joeconway(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: SYSTEM_USER reserved word implementation |
Date: | 2022-09-26 13:09:40 |
Message-ID: | ea078f3a-29d9-9983-6bf8-35aea17d40bb@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Drouvot, Bertrand | 2022-09-26 13:28:16 | Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back) |
Previous Message | torikoshia | 2022-09-26 13:04:36 | Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features) |