Amit, * Amit Langote (langote_amit...@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
signature.asc
Description: Digital signature
