Re: SYSTEM_USER reserved word implementation

Hi,

On 9/28/22 5:28 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2022 at 03:38:49PM -0700, Jacob Champion wrote:
>> On 9/26/22 06:29, Drouvot, Bertrand wrote:
>> Since there are only internal clients to the API, I'd argue this makes
>> more sense as an Assert(authn_id != NULL), but I don't think it's a
>> dealbreaker.
> 
> Using an assert() looks like a good idea from here.  If this is called
> with a NULL authn, this could reflect a problem in the authentication
> logic.
> 

Agree, thanks for pointing out.

>>> As far the assertion failure mentioned by Michael when moving the
>>> SVFOP_SYSTEM_USER from NAMEOID to TEXTOID: V4 is assuming that it is
>>> safe to force the collation to C_COLLATION_OID for SQLValueFunction
>>> having a TEXT type, but I would be happy to also hear your thoughts
>>> about it.
>>
>> Unfortunately I don't have much to add here; I don't know enough about
>> the underlying problems.
> 
> I have been looking at that, and after putting my hands on that this
> comes down to the facility introduced in 40c24bf.  So, I think that
> we'd better use COERCE_SQL_SYNTAX so as there is no need to worry
> about the shortcuts this patch is trying to use with the collation
> setup.

Nice!

>  And there are a few tests for get_func_sql_syntax() in
> create_view.sql.  Note that this makes the patch slightly shorter, and
> simpler.
>

Agree that it does look simpler that way and that making use of 
COERCE_SQL_SYNTAX does looks like a better approach. Nice catch!


> The docs still mentioned "name", and not "text".
> 

Oups, thanks for pointing out.

I had a look at v5 and it does look good to me.

Regards,

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