On Wed, Sep 07, 2022 at 08:48:43AM -0700, Jacob Champion wrote:
> 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.
Yeah, this had better use a <link>.
>> + /* 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).
You have mentioned that a couple of months ago if I recall correctly,
and we pass down an enum value.
> 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?
Yeah, it should. There is actually a second and much deeper issue
here, in the shape of a collation problem. See the assertion failure
in exprSetCollation(), because we expect SQLValueFunction nodes to
return a name or a non-collatable type. However, for this case, we'd
require a text to get rid of the 63-character limit, and that's
a collatable type. This reminds me of the recent thread to work on
getting rid of this limit for the name type..
--
Michael
