On 10/27/14 11:40 AM, Stephen Frost wrote:
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
>>> As I started looking at this, there are multiple other places where
>>> these types of error messages occur (opclasscmds.c, user.c,
>>> postinit.c, miscinit.c are just a few), not just around the changes in
>>> this patch. If we change them in one place, wouldn't it be best to
>>> change them in the rest? If that is the case, I'm afraid that might
>>> distract from the purpose of this patch. Perhaps, if we want to
>>> change them, then that should be submitted as a separate patch?
>>
>> Yeah. I'm just saying that maybe this patch should adopt whatever
>> wording we agree to, not that we need to change other places. On the
>> other hand, since so many other places have adopted the different
>> wording, maybe there's a reason for it and if so, does anybody know what
>> it is. But I have to say that it does look inconsistent to me.
>
> Updated patch attached. Comments welcome.
The changes in
src/backend/commands/alter.c
src/backend/commands/foreigncmds.c
src/backend/commands/tablecmds.c
src/backend/commands/typecmds.c
are OK, because the superuser() calls are redundant, and cleaning this
up is clearly in line with planned future work.
I suggest moving the rest of the changes into separate patches.
The error message wording changes might well be worth considering, but
there are tons more "must be $someone to do $something" error messages,
and changing two of them isn't making anything more or less consistent.
The ha*_something_privilege() changes are also not very consistent.
We already have have_createrole_privilege(), which does include a
superuser check, and you add has_replication_privilege() with a
superuser check, but has_catupdate_privilege() and
has_inherit_privilege() don't include a superuser check. That's clearly
a mess.
Btw., why rename have_createrole_privilege()?
Also, your patch has spaces between tabs. Check for whitespace errors
with git.
