Amit,
* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> Currently in CreateUserMapping():
>
> /* Additional check to protect reserved role names */
> check_rolespec_name(stmt->user,
> "Cannot specify reserved role as mapping user.");
>
> User mapping terminology is not that clear to me really but how does the
> following sound as detail message:
>
> "Cannot create mapping for reserved roles" or "Cannot create reserved role
> mapping"
I'm open to changing that, though I don't think the existing terminology
is all that bad either.
> Also then, are checks for reserved role specification in
> AlterUserMapping() and RemoveUserMapping() really necessary?
>
> /* Additional check to protect reserved role names */
> check_rolespec_name(stmt->user,
> "Cannot alter reserved role mapping user.");
>
> /* Additional check to protect reserved role names */
> check_rolespec_name(stmt->user,
> "Cannot remove reserved role mapping user.");
That was intentional as I was covering all cases where
get_rolespec_oid/tuple() are used to convert rolename->OID for any
purpose, as a method of trying to ensure coverage of all code paths.
Creation of user mappings are different from most objects in that they
are explicitly created, rather than created with the implicit assumption
that the creator owns them, which is why this is a bit different than
the other cases.
> Messages output in those cases are:
>
> ERROR: role "pg_signal_backend" is reserved
> DETAIL: Cannot alter reserved role mapping user.
>
> ERROR: role "pg_signal_backend" is reserved
> DETAIL: Cannot remove reserved role mapping user.
>
> Whereas, the following would seem more natural:
>
> ERROR: user mapping "pg_signal_backend" does not exist for the server
Perhaps. I can almost imagine a case where we ship a default role which
has a pre-defined mapping to the "local" server for the purpose of
accessing another database through postgres_fdw (which has since become
included by default, similar to plpgsql). Yeah, that's a reallllly long
stretch, but I'm not really a fan of remove those checks from this code
path.
On the other hand, perhaps we could move those checks to after the
lookup for the user mapping, thus getting the above error message
(unless we do end up with such a mapping in PostgreSQL 16) and not
leaving the paths un-checked.
Thanks!
Stephen