Re: superuser() shortcuts

All,

We, as a community, have gotten flak from time-to-time about the
superuser role. We also tend to wish to avoid unnecessary
optimization as it complicates the code base and makes folks reviewing
the code wonder at the exceptions.

As such, I wonder at a few of superuser() checks we have today
which appear to be entirely superfluous, specifically:

replication/logical/logicalfuncs.c
check_permissions()

My 2c about this function is that it should be completely removed
and the place where it's checked replaced with just the
'has_rolreplication' call and error. It's only called in one
place and it'd be a simple one-liner anyway. As for
has_rolreplication, I don't understand why it's in miscinit.c when
the rest of the has_* set is in acl.c.

replication/slotfuncs.c - more or less the same

commands/alter.c
AlterObjectOwner_internal()
There's a shortcut here for superuser() that appears entirely
redundant as the immediately following 'has_privs_of_role()' will
return true for all superuser, as will the later
check_is_member_of_role() call, and the pg_namespace_aclcheck will
also return true. Perhaps I'm missing something, but why isn't
this superuser() check completely redundant and possible not ideal
(what if Anum_name is valid but NULL after all..?).

commands/tablecmds.c
ATExecChangeOwner()
The superuser check here looks to just be avoiding extra
permission checks, but that could change and we might eventually
end up in a situation similar to above where other checks are
happening (possibly to avoid a crash) but don't end up happenning
for superuser by mistake. I don't feel like table owner changes
happen so often that we need to avoid a couple extra function
calls and so I would recommend ripping out the explicit
superuser() check here.

commands/typecmds.c
AlterTypeOwner()
More-or-less the same as above.

commands/foreigncmds.c
AlterForeignServerOwner_internal()
Ditto.

Removing these design patterns may also help to avoid ending up with
more of them in the future as folks copy and/or crib off of what we've
already done to implement their features...

Thanks!

Stephen

Stephen,

We, as a community, have gotten flak from time-to-time about the
> superuser role. We also tend to wish to avoid unnecessary
> optimization as it complicates the code base and makes folks reviewing
> the code wonder at the exceptions.
>

I have attached a patch for consideration/discussion that addresses these
items. I have based it off of current master (0c013e0). I have done some
cursory testing including check-world with success.

> My 2c about this function is that it should be completely removed
> and the place where it's checked replaced with just the
> 'has_rolreplication' call and error. It's only called in one
> place and it'd be a simple one-liner anyway. As for
> has_rolreplication, I don't understand why it's in miscinit.c when
> the rest of the has_* set is in acl.c.
>

With all the other pg_has_* functions being found in acl.c I agree that it
would seem odd that this (or any other related function) would be found
elsewhere. Since aclchk.c also has a couple of functions that follow the
pattern of "has_<priv>_privilege", I moved this function there, for now, as
well as modified it to include superuser checks as they were not previously
there. The only related function I found in acl.c was "has_rolinherit"
which is a static function. :-/ There is also a static function
"has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
would be a more appropriate location for these functions, though in some of
the above cases, it might require exposing them (I'm not sure that I see
any harm in doing so).

Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
with the following pattern might be a step in the right direction?

* has_createrole_privilege
* has_bypassrls_privilege
* has_inherit_privilege
* has_catupdate_privilege
* has_replication_privilege
* has_???_privilege

In either case, I think following a "convention" on the naming of these
functions (unless there is semantic reason not to) would also help to
reduce any confusion or head scratching.

Removing these design patterns may also help to avoid ending up with
> more of them in the future as folks copy and/or crib off of what we've
> already done to implement their features...
>

I agree.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Brightwell, Adam (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> > We, as a community, have gotten flak from time-to-time about the
> > superuser role. We also tend to wish to avoid unnecessary
> > optimization as it complicates the code base and makes folks reviewing
> > the code wonder at the exceptions.
>
> I have attached a patch for consideration/discussion that addresses these
> items. I have based it off of current master (0c013e0). I have done some
> cursory testing including check-world with success.

Thanks! Please add it to the next commitfest.

> With all the other pg_has_* functions being found in acl.c I agree that it
> would seem odd that this (or any other related function) would be found
> elsewhere. Since aclchk.c also has a couple of functions that follow the
> pattern of "has_<priv>_privilege", I moved this function there, for now, as
> well as modified it to include superuser checks as they were not previously
> there. The only related function I found in acl.c was "has_rolinherit"
> which is a static function. :-/ There is also a static function
> "has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
> would be a more appropriate location for these functions, though in some of
> the above cases, it might require exposing them (I'm not sure that I see
> any harm in doing so).

I don't think has_rolinherit or has_rolcatupdate really need to move and
it seems unlikely that they'd be needed from elsewhere.. Is there a
reason you think they'd need to be exposed? I've not looked at the
patch at all though, perhaps that makes it clear.

> Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
> with the following pattern might be a step in the right direction?
>
> * has_createrole_privilege
> * has_bypassrls_privilege

These are already in the right place, right?

> * has_inherit_privilege
> * has_catupdate_privilege

These probably don't need to move as they're only used in the .c files
that they're defined in (unless there's a reason that needs to change).

> * has_replication_privilege

This is what I was on about, so I agree that it should be created. ;)

> * has_???_privilege

Right, other things might be 'has_backup_privilege', for things like
pg_start/stop_backup and friends.

> In either case, I think following a "convention" on the naming of these
> functions (unless there is semantic reason not to) would also help to
> reduce any confusion or head scratching.

Agreed.

Thanks!

Stephen

Stephen,

Thanks! Please add it to the next commitfest.

Sounds good. I'll update the patch and add accordingly.

> I don't think has_rolinherit or has_rolcatupdate really need to move and
> it seems unlikely that they'd be needed from elsewhere.. Is there a
> reason you think they'd need to be exposed? I've not looked at the
> patch at all though, perhaps that makes it clear.

There is no reason to expose them (at this point in time) other than
consolidation.

> * has_createrole_privilege
> > * has_bypassrls_privilege
> These are already in the right place, right?
>

If aclchk.c is the right place, then yes. :-)

> > * has_inherit_privilege
> > * has_catupdate_privilege
>
> These probably don't need to move as they're only used in the .c files
> that they're defined in (unless there's a reason that needs to change).
>

Correct, though, I don't see any reason for them to move other than
attempting to consolidate them.

> > * has_???_privilege
>
> Right, other things might be 'has_backup_privilege', for things like
> pg_start/stop_backup and friends.

Correct.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

All,

> Thanks! Please add it to the next commitfest.
>
>
> Sounds good. I'll update the patch and add accordingly.
>

Attached is an updated patch.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Brightwell, Adam wrote:
> All,
>
>
> > Thanks! Please add it to the next commitfest.
> >
> >
> > Sounds good. I'll update the patch and add accordingly.
> >
>
> Attached is an updated patch.

I noticed something strange while perusing this patch, but the issue
predates the patch. Some messages say "must be superuser or replication
role to foo", but our longstanding practice is to say "permission denied
to foo". Why do we have this inconsistency? Should we remove it? If
we do want to keep the old wording this patch should use "permission
denied to" in the places that it touches.

Other than that, since we already agreed that it's something we want,
the only comment I have about this patch is an empty line in variable
declarations here which should be removed:

> diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
> new file mode 100644
> index c9a9baf..ed89b23
> *** a/src/backend/commands/alter.c

> --- 807,848 ----
> bool *nulls;
> bool *replaces;
>
> ! AclObjectKind aclkind = get_object_aclkind(classId);
> !

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,

> I noticed something strange while perusing this patch, but the issue
> predates the patch. Some messages say "must be superuser or replication
> role to foo", but our longstanding practice is to say "permission denied
> to foo". Why do we have this inconsistency? Should we remove it? If
> we do want to keep the old wording this patch should use "permission
> denied to" in the places that it touches.

If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?

Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:

errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

>> I noticed something strange while perusing this patch, but the issue
>> predates the patch. Some messages say "must be superuser or replication
>> role to foo", but our longstanding practice is to say "permission denied
>> to foo". Why do we have this inconsistency? Should we remove it? If
>> we do want to keep the old wording this patch should use "permission
>> denied to" in the places that it touches.
>
> If we were to make it consistent and use the old wording, what do you
> think about providing an "errhint" as well?
>
> Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
>
> errmsg - "permission denied to create physical replication slot"
> errhint - "You must be superuser or replication role to use replication slots."

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?

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Brightwell, Adam wrote:

> > If we were to make it consistent and use the old wording, what do you
> > think about providing an "errhint" as well?
> >
> > Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
> >
> > errmsg - "permission denied to create physical replication slot"
> > errhint - "You must be superuser or replication role to use replication slots."

Sure.

> 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.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/23/14, 6:23 PM, Alvaro Herrera wrote:
> Brightwell, Adam wrote:
>
>>> If we were to make it consistent and use the old wording, what do you
>>> think about providing an "errhint" as well?
>>>
>>> Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
>>>
>>> errmsg - "permission denied to create physical replication slot"
>>> errhint - "You must be superuser or replication role to use replication slots."
>
> Sure.
>
>> 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.

Keep in mind that originally the ONLY special permissions "thing" we had was rolsuper. Now we also have replication, maybe some others, and there's discussion of adding a special "backup role".

In other words, the situation is a lot more complex than it used to be. So if cleanup is done here, it would be best to try and account for this new stuff.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> Brightwell, Adam wrote:
> > > If we were to make it consistent and use the old wording, what do you
> > > think about providing an "errhint" as well?
> > >
> > > Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
> > >
> > > errmsg - "permission denied to create physical replication slot"
> > > errhint - "You must be superuser or replication role to use replication slots."
>
> Sure.

Sounds reasonable to me and looks to be in-line with wording elsewhere.

> > 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.

I agree that this patch should adopt the wording agreed to above and
that it's 'more similar' to most of the usage in the backend. As for
the general question, I'd suggest we add to the error message policy
that more-or-less anything with errcode(ERRCODE_INSUFFICIENT_PRIVILEGE)
have errmsg("permission denied to X") with any comments about 'must be a
superuser' or similar relegated to errhint(). We could add that as a
TODO and have more junior developers review these cases and come up with
patches (or argue for cases where they feel deviation is warranted).

Either way, that larger rework isn't for this patch and since you seem
happy with it (modulo the change above), I'm going to make that change,
do my own review, and move foward with it unless someone else wants to
speak up.

Thanks!

Stephen

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)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.

Thanks!

Stephen

All,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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.

Looking over this again, I had another thought about it- given that this
changes the error messages returned for replication slots, which are new
in 9.4, should it be back-patched to 9.4? Otherwise we'll put 9.4
out and then immediately change these error messages in 9.5.

That said, it seems likely we'll be doing a more thorough review and
update of error messages for 9.5 (if others agree with my up-thread
proposal), such that these changes would be minor additional ones.

Thoughts? I don't have a preference either way, which makes me lean
towards not messing with 9.4, but wanted to bring it up.

Thanks!

Stephen

On 2014-10-28 09:43:35 -0400, Stephen Frost wrote:
> All,
>
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> > * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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.
>
> Looking over this again, I had another thought about it- given that this
> changes the error messages returned for replication slots, which are new
> in 9.4, should it be back-patched to 9.4? Otherwise we'll put 9.4
> out and then immediately change these error messages in 9.5.

-1.

For one I'm less than convinced that the new messages are an
improvement. They seem to be more verbose without a corresponding
improvement in clarity.

For another I don't see any need to rush this into 9.4.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> For one I'm less than convinced that the new messages are an
> improvement. They seem to be more verbose without a corresponding
> improvement in clarity.

The goal with the changes is to improve consistency of messaging. These
messages are not at all consistent today. Personally, I like the idea
of being clear in the main errmsg() that these are permission denied
errors, but we could go the other way and change all the existing
messages which say 'permission denied' to only say 'you have to be
superuser or have X' instead and expect folks to realize it's a
permission denied error from the SQL error code..

> For another I don't see any need to rush this into 9.4.

Ok.

Thanks!

Stephen

On 10/27/14 11:40 AM, Stephen Frost wrote:
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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.

Thanks for looking at this patch.

> I suggest moving the rest of the changes into separate patches.
>

Hmmm... perhaps the following?

* superuser-cleanup - contains above mentioned superuser shortcuts only.
* has_privilege-cleanup - contains has_*_priviledge cleanup only.

Would that also require a separate commitfest entry?

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.
>

Good catch. Though, according to the documentation, not even superuser is
allowed to bypass CATUPDATE.

http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html.

However, I can't think of a reason why "inherit" wouldn't need the
superuser check. Obviously superuser is considered a member of every role,
but is there a reason that a superuser would not be allowed to bypass
this? I only ask because it did not have a check previously, so I figure
there might have been a good reason for it?

Btw., why rename have_createrole_privilege()?
>

Well, actually it wasn't necessarily a rename. It was a removal of that
function all together as all it did was simply return the result of
"has_createrole_privilege". That seemed rather redundant and unnecessary,
IMO.

Also, your patch has spaces between tabs. Check for whitespace errors
> with git.
>

Yikes.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Attached is two separate patches to address previous
comments/recommendations:

* superuser-cleanup-shortcuts_11-5-2014.patch
* has_privilege-cleanup_11-5-2014.patch

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On 11/5/14 5:10 PM, Adam Brightwell wrote:
> Attached is two separate patches to address previous
> comments/recommendations:
>
> * superuser-cleanup-shortcuts_11-5-2014.patch

Seeing that the regression tests had to be changed in this patch
indicates that there is a change of functionality, even though this
patch was previously discussed as merely an internal cleanup. So either
there is a user-facing change, which would need to be documented (or at
least mentioned, discussed, and dismissed as minor), or there is a fault
in the tests, which should be fixed first independently.

Which makes me wonder whether the other changes are indeed without
effect or just not covered by tests.

> * has_privilege-cleanup_11-5-2014.patch

I don't really see the merit of this patch. A "cleanup" patch that adds
more code than it removes isn't really a cleanup. If you want to make
the APIs consistent, then let's make a patch for that. If you want to
make the error messages consistent, then let's make a patch for that.
There is other work going on replacing these role attributes with
something more general, so maybe this cleanup should be done as part of
that other work.

On 2014-11-05 17:10:17 -0500, Adam Brightwell wrote:
> Attached is two separate patches to address previous
> comments/recommendations:
>
> * superuser-cleanup-shortcuts_11-5-2014.patch
> * has_privilege-cleanup_11-5-2014.patch
>
> -Adam
>
> --
> Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
> Database Engineer - www.crunchydatasolutions.com

> diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
> new file mode 100644
> index 212fd1d..f011955
> *** a/contrib/test_decoding/expected/permissions.out
> --- b/contrib/test_decoding/expected/permissions.out
> *************** RESET ROLE;
> *** 54,66 ****
> -- plain user *can't* can control replication
> SET ROLE lr_normal;
> SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
> ! ERROR: must be superuser or replication role to use replication slots
> INSERT INTO lr_test VALUES('lr_superuser_init');
> ERROR: permission denied for relation lr_test
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> ! ERROR: must be superuser or replication role to use replication slots
> SELECT pg_drop_replication_slot('regression_slot');
> ! ERROR: must be superuser or replication role to use replication slots
> RESET ROLE;
> -- replication users can drop superuser created slots
> SET ROLE lr_superuser;
> --- 54,69 ----
> -- plain user *can't* can control replication
> SET ROLE lr_normal;
> SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
> ! ERROR: permission denied to use replication slots
> ! HINT: You must be superuser or replication role to use replication slots.
> INSERT INTO lr_test VALUES('lr_superuser_init');
> ERROR: permission denied for relation lr_test
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> ! ERROR: permission denied to use replication slots
> ! HINT: You must be superuser or replication role to use replication slots.
> SELECT pg_drop_replication_slot('regression_slot');
> ! ERROR: permission denied to use replication slots
> ! HINT: You must be superuser or replication role to use replication slots.
> RESET ROLE;
> -- replication users can drop superuser created slots
> SET ROLE lr_superuser;

I still think this change makes the error message more verbose, without
any win in clarity.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> I still think this change makes the error message more verbose, without
> any win in clarity.

Can we agree that there should be consistency? I'm not really
particular about which way we go with the specific wording (suggestions
welcome..) but the inconsistency should be dealt with.

Thanks,

Stephen

Peter,

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 11/5/14 5:10 PM, Adam Brightwell wrote:
> > Attached is two separate patches to address previous
> > comments/recommendations:
> >
> > * superuser-cleanup-shortcuts_11-5-2014.patch
>
> Seeing that the regression tests had to be changed in this patch
> indicates that there is a change of functionality, even though this
> patch was previously discussed as merely an internal cleanup. So either
> there is a user-facing change, which would need to be documented (or at
> least mentioned, discussed, and dismissed as minor), or there is a fault
> in the tests, which should be fixed first independently.

It was brought up for discussion- see
http://www.postgresql.org/message-id/20141015052259.GG28859@tamriel.snowman.net

Specifically:
---------------
One curious item to note is that the
current if(!superuser()) {} block approach has masked an inconsistency
in at least the FDW code which required a change to the regression
tests- namely, we normally force FDW owners to have USAGE rights on
the FDW, but that was being bypassed when a superuser makes the call.
I seriously doubt that was intentional. I won't push back if someone
really wants it to stay that way though.
---------------

No one mentioned any concerns with it (and three people replied), so I'm
inclined to think it's an agreeable change. Adam probably didn't
mention it with this patch simply because it had already been brought
up.

> Which makes me wonder whether the other changes are indeed without
> effect or just not covered by tests.
>
> > * has_privilege-cleanup_11-5-2014.patch
>
> I don't really see the merit of this patch. A "cleanup" patch that adds
> more code than it removes isn't really a cleanup. If you want to make
> the APIs consistent, then let's make a patch for that. If you want to
> make the error messages consistent, then let's make a patch for that.
> There is other work going on replacing these role attributes with
> something more general, so maybe this cleanup should be done as part of
> that other work.

Perhaps 'cleanup' is just an overloaded term. The patch is to make the
APIs and the error messages consistent. I'm amazed at how much
discussion and work is going into these exceptionally minor changes
which have been already broken out of the larger and far more
interesting work (imv anyway). Having two patches, one to simply move
the checks around and then another to make the error messages in those
checks consistent, which will naturally end up depending on each other,
strikes me as overkill, but we can certainly do it.

Andres raised a concern about the specific error message wording (which
was intended to just make it more consistent with the rest of our
permission-check error messages..), are there any other opinions on the
wording? Would be great to get feedback on that..

Thanks!

Stephen

All,

> It was brought up for discussion- see
>
> http://www.postgresql.org/message-id/20141015052259.GG28859@tamriel.snowman.net
>
> Specifically:
> ---------------
> One curious item to note is that the
> current if(!superuser()) {} block approach has masked an inconsistency
> in at least the FDW code which required a change to the regression
> tests- namely, we normally force FDW owners to have USAGE rights on
> the FDW, but that was being bypassed when a superuser makes the call.
> I seriously doubt that was intentional. I won't push back if someone
> really wants it to stay that way though.
> ---------------
>
> No one mentioned any concerns with it (and three people replied), so I'm
> inclined to think it's an agreeable change. Adam probably didn't
> mention it with this patch simply because it had already been brought
> up.

Yes, this is correct, I was under the impression that this has already been
addressed. Also, I thought it is a relatively benign change and perhaps
even one for the better. With that said, I'll certainly leave it as is if
that is the consensus.

> > Which makes me wonder whether the other changes are indeed without
> > effect or just not covered by tests.
> >
> > > * has_privilege-cleanup_11-5-2014.patch
> >
> > I don't really see the merit of this patch. A "cleanup" patch that adds
> > more code than it removes isn't really a cleanup. If you want to make
> > the APIs consistent, then let's make a patch for that. If you want to
> > make the error messages consistent, then let's make a patch for that.
>

Fair enough. I think we all agree that fixing the superuser shortcuts are
a relevant and welcome change (at least that is the sense I get). So, how
about for the time being, we table the "consistent API and error messages"
and focus solely on the shortcuts? If that is favorable, then I have
attached a patch to address those changes.

> There is other work going on replacing these role attributes with
> > something more general, so maybe this cleanup should be done as part of
> > that other work.
>

I agree and given the work that has already been done toward that effort I
think that would perhaps be the better approach to addressing them.

Perhaps 'cleanup' is just an overloaded term. The patch is to make the
> APIs and the error messages consistent. I'm amazed at how much
> discussion and work is going into these exceptionally minor changes
> which have been already broken out of the larger and far more
> interesting work (imv anyway). Having two patches, one to simply move
> the checks around and then another to make the error messages in those
> checks consistent, which will naturally end up depending on each other,
> strikes me as overkill, but we can certainly do it.
>

Agreed, but will certainly do whatever is necessary to keep these changes
moving forward. Though, I think the attached patch mitigates any need to
break these changes out further.

Andres raised a concern about the specific error message wording (which
> was intended to just make it more consistent with the rest of our
> permission-check error messages..), are there any other opinions on the
> wording? Would be great to get feedback on that..
>

Agreed, I would certainly be willing to move these changes forward as a
separate effort, but without more specific recommendations beyond what has
already been discussed and proposed I'm at a bit of a loss on where to take
it. I'm all ears on this one and would certainly appreciate any feedback

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On 2014-11-21 10:12:40 -0500, Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > I still think this change makes the error message more verbose, without
> > any win in clarity.
>
> Can we agree that there should be consistency?

Consistency with what? Are you thinking of the messages in
aclck.c:no_priv_msg? I don't think that's really comparable. A
permission denied on a relation is much easier to understand than
replication permissions and such.

It'd surely not be better if pg_basebackup would a error message bar
actually helpful information. Btw, the replication permission use in
postinit.c isn't related to slots.

> I'm not really particular about which way we go with the specific
> wording (suggestions welcome..) but the inconsistency should be dealt
> with.

Meh.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-11-21 10:12:40 -0500, Stephen Frost wrote:
> > * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > > I still think this change makes the error message more verbose, without
> > > any win in clarity.
> >
> > Can we agree that there should be consistency?
>
> Consistency with what? Are you thinking of the messages in
> aclck.c:no_priv_msg? I don't think that's really comparable. A
> permission denied on a relation is much easier to understand than
> replication permissions and such.

The discussion around wording started here, I believe:

20141022231834(dot)GA1587(at)alvin(dot)alvh(dot)no-ip(dot)org

Perhaps more to your question though, all checks of
'have_createdb_privilege' return 'permission denied to' style errors,
'have_createrole_privilege' returns 'permission denied' style for all
except where it returns the more specific 'must have admin option',
the 'has_rolcatupdate' check returns 'permission denied', and the
'has_bypassrls_privilege' check returns 'insufficient privilege' (note:
I'm in favor of changing that to use 'permission denied' instead too).

With regard to ereport() calls which return
ERRCODE_INSUFFICIENT_PRIVILEGE, things are pretty mixed up. Some places
places say 'permission denied to' and then have 'must be superuser' as a
hint while others just say 'must be superuser' and then others are just
'permission denied' (such as aclchk.c:no_priv_msg).

> It'd surely not be better if pg_basebackup would a error message bar
> actually helpful information.

ENOPARSE. I certainly agree that we want useful information to be
returned, in general..

> Btw, the replication permission use in
> postinit.c isn't related to slots.

Err, no, of course not, that should still be referring to starting
walsender.

Thanks!

Stephen

On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I'm not really particular about which way we go with the specific
>> wording (suggestions welcome..) but the inconsistency should be dealt
>> with.
>
> Meh.

+1 for "meh". I don't mind making things consistent if it can be done
while maintaining or improving the absolute quality of those error
messages -- but if the changes involve a loss of detail, or moving
information that used to be in the main error message into the detail,
then I don't think it's worth it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> I'm not really particular about which way we go with the specific
> >> wording (suggestions welcome..) but the inconsistency should be dealt
> >> with.
> >
> > Meh.
>
> +1 for "meh". I don't mind making things consistent if it can be done
> while maintaining or improving the absolute quality of those error
> messages -- but if the changes involve a loss of detail, or moving
> information that used to be in the main error message into the detail,
> then I don't think it's worth it.

Doesn't that argument then apply to the other messages which I pointed
out in my follow-up to Andres, where the detailed info is in the hint
and the main error message is essentially 'permission denied'? Also, if
we're going to make these error-messages related to role attributes be
'you need role attribute X', should we consider doing the same for the
regular 'permission denied' error messages?

I can understand the arguments about loss of detail or having the detail
in the hint instead of the error message. I don't understand why we'd
want the messaging to be inconsistent.

Thanks,

Stephen

On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > >> I'm not really particular about which way we go with the specific
> > >> wording (suggestions welcome..) but the inconsistency should be dealt
> > >> with.
> > >
> > > Meh.
> >
> > +1 for "meh". I don't mind making things consistent if it can be done
> > while maintaining or improving the absolute quality of those error
> > messages -- but if the changes involve a loss of detail, or moving
> > information that used to be in the main error message into the detail,
> > then I don't think it's worth it.
>
> Doesn't that argument then apply to the other messages which I pointed
> out in my follow-up to Andres, where the detailed info is in the hint
> and the main error message is essentially 'permission denied'?

A permission error on a relation is easier to understand than one
for some abstract 'replication' permission.

> Also, if we're going to make these error-messages related to role
> attributes be 'you need role attribute X', should we consider doing
> the same for the regular 'permission denied' error messages?

> I can understand the arguments about loss of detail or having the detail
> in the hint instead of the error message. I don't understand why we'd
> want the messaging to be inconsistent.

I'm not generally against making them more consistent. I'm vehemently
against making them consistent by making them worse. And the suggested
changes are worse.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
> > Doesn't that argument then apply to the other messages which I pointed
> > out in my follow-up to Andres, where the detailed info is in the hint
> > and the main error message is essentially 'permission denied'?
>
> A permission error on a relation is easier to understand than one
> for some abstract 'replication' permission.

The more I consider this and review the error message reporting policy,
the more I feel that the original coding was wrong and that this change
*is* the correct one to make.

Even the example in the documentation makes a point of having the
errcode() and the errmsg() match up:

ereport(ERROR,
(errcode(ERRCODE_DIVISION_BY_ZERO),
errmsg("division by zero")));

The second example is similar:

ereport(ERROR,
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
errmsg("function %s is not unique",
func_signature_string(funcname, nargs,
NIL, actual_arg_types)),
errhint("Unable to choose a best candidate function. "
"You might need to add explicit typecasts.")));

Further, the comments around many of the places which use
ACLCHECK_NOT_OWNER (which also uses the 'must be/have X' style) are
"permissions check for X", and what's more, we've had things go from one
to the other previously even though the user action wasn't changing-
specifically TRUNCATE used to be owner-only and was changed to be
grantable.

The reason for the error *is* permission denied, hence the errcode().
The detail is that the permission is reserved to the owner, or to roles
which have a given attribute. The error isn't "must by X". I'm of the
opinion at this point that we should change the ACLCHECK_NOT_OWNER
branch under aclcheck_error() to match what is proposed by this patch-
have a 'permission denied' error for whatever the action is and then
have errdetail of 'not_owner_msg[objectkind]'.

I don't think we need to include errdetail() for the ACLCHECK_NO_PRIV
case as those permissions are part of the normal GRANT/REVOKE privilege
system. The detail is warranted when we're talking about things which
are outside of the normal privilege system.

If it isn't a permission denied error then it shouldn't be using
ERRCODE_INSUFFICIENT_PRIVILEGE.

Thanks,

Stephen

Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
> > > Doesn't that argument then apply to the other messages which I pointed
> > > out in my follow-up to Andres, where the detailed info is in the hint
> > > and the main error message is essentially 'permission denied'?
> >
> > A permission error on a relation is easier to understand than one
> > for some abstract 'replication' permission.
>
> The more I consider this and review the error message reporting policy,
> the more I feel that the original coding was wrong and that this change
> *is* the correct one to make.

+1. I don't care for the idea of "not moving from main error message to
errdetail" -- the rationale seems to be that errdetail might be hidden,
lost, or otherwise not read by the user; if that is so, why do we have
errdetail in the first place? We might as well just move all the
errdetails into the main message, huh?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Stephen Frost wrote:
>> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
>>> On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
>>>> Doesn't that argument then apply to the other messages which I pointed
>>>> out in my follow-up to Andres, where the detailed info is in the hint
>>>> and the main error message is essentially 'permission denied'?

>> The more I consider this and review the error message reporting policy,
>> the more I feel that the original coding was wrong and that this change
>> *is* the correct one to make.

> +1. I don't care for the idea of "not moving from main error message to
> errdetail" -- the rationale seems to be that errdetail might be hidden,
> lost, or otherwise not read by the user; if that is so, why do we have
> errdetail in the first place? We might as well just move all the
> errdetails into the main message, huh?

I might be overlooking some corner case, but most of our permission-type
error messages are not just "permission denied" full stop; they're more
like "permission denied for <object>". So I think it'd be sensible for
the main error message to be something like "permission denied for
replication", and then additional info in errdetail if that seems
warranted. But "permission denied" all by itself seems too vague
to be useful --- even the simplest SQL command usually has multiple
ways that it could conceivably trip over a permissions restriction.
The concept of errdetail has always been "extra info that might be
helpful", not "information you *must* have to have any hope of
understanding the problem".

In the context at hand, I think most of the messages in question are
currently phrased like "must be superuser to do X". I'd be fine with
changing that to "permission denied to do X", but not to just
"permission denied".

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> In the context at hand, I think most of the messages in question are
> currently phrased like "must be superuser to do X". I'd be fine with
> changing that to "permission denied to do X", but not to just
> "permission denied".

Apologies for the terseness of my (earlier) reply. This is exactly what
I'm suggesting. What was in the patch was this change:

! ERROR: must be superuser or replication role to use replication slots

---

! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.

On reflection, perhaps it should be an errdetail() message instead of an
errhint() message, but the complaint levied against the change was
making it be 'permission denied to X' and moving the 'must be superuser'
out of the errmsg().

Thanks!

Stephen

On 2014-11-26 11:53:40 -0300, Alvaro Herrera wrote:
> Stephen Frost wrote:
> > * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > > On 2014-11-26 08:33:10 -0500, Stephen Frost wrote:
> > > > Doesn't that argument then apply to the other messages which I pointed
> > > > out in my follow-up to Andres, where the detailed info is in the hint
> > > > and the main error message is essentially 'permission denied'?
> > >
> > > A permission error on a relation is easier to understand than one
> > > for some abstract 'replication' permission.
> >
> > The more I consider this and review the error message reporting policy,
> > the more I feel that the original coding was wrong and that this change
> > *is* the correct one to make.
>
> +1. I don't care for the idea of "not moving from main error message to
> errdetail" -- the rationale seems to be that errdetail might be hidden,
> lost, or otherwise not read by the user; if that is so, why do we have
> errdetail in the first place? We might as well just move all the
> errdetails into the main message, huh?

That rationale is imo bogus. The actual error message should be helpful
and informative - only if the error would be to verbose it should go
into the detail. It's not exactly unimportant to know why something was
prohibited. It's not like this is any form of new precedent - *loads* of
errmsg() style of messages have more information than the static string.

I don't see how you read the contrary from the guidelines:
> The primary message should be short, factual, and avoid reference to
> implementation details such as specific function names. "Short" means
> "should fit on one line under normal conditions". Use a detail message
> if needed to keep the primary message short, or if you feel a need to
> mention implementation details such as the particular system call that
> failed. Both primary and detail messages should be factual. Use a hint
> message for suggestions about what to do to fix the problem, especially
> if the suggestion might not always be applicable. It's quite common
> that you'll only see the actual error message in logs and displayed
> error messages.

That seems to give credence to *my* position, not yours.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> I don't see how you read the contrary from the guidelines:
> > The primary message should be short, factual, and avoid reference to
> > implementation details such as specific function names. "Short" means
> > "should fit on one line under normal conditions". Use a detail message
> > if needed to keep the primary message short, or if you feel a need to
> > mention implementation details such as the particular system call that
> > failed. Both primary and detail messages should be factual. Use a hint
> > message for suggestions about what to do to fix the problem, especially
> > if the suggestion might not always be applicable. It's quite common
> > that you'll only see the actual error message in logs and displayed
> > error messages.

Having to be the owner of a relation to TRUNCATE it was an
implementation detail, and one which changed back in 2008.

Needing to have the replication role attribute is an implementation
detail. It's not the error- the error is *permission denied to do X*.

Thanks,

Stephen

On 2014-11-26 10:18:20 -0500, Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > I don't see how you read the contrary from the guidelines:
> > > The primary message should be short, factual, and avoid reference to
> > > implementation details such as specific function names. "Short" means
> > > "should fit on one line under normal conditions". Use a detail message
> > > if needed to keep the primary message short, or if you feel a need to
> > > mention implementation details such as the particular system call that
> > > failed. Both primary and detail messages should be factual. Use a hint
> > > message for suggestions about what to do to fix the problem, especially
> > > if the suggestion might not always be applicable. It's quite common
> > > that you'll only see the actual error message in logs and displayed
> > > error messages.
>
> Having to be the owner of a relation to TRUNCATE it was an
> implementation detail, and one which changed back in 2008.
>
> Needing to have the replication role attribute is an implementation
> detail. It's not the error- the error is *permission denied to do X*.

That's just plain absurd. If the user needs to explicitly configure that
permission it's damned sure not just a implementation detail.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-11-26 10:18:20 -0500, Stephen Frost wrote:
> > * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > > I don't see how you read the contrary from the guidelines:
> > > > The primary message should be short, factual, and avoid reference to
> > > > implementation details such as specific function names. "Short" means
> > > > "should fit on one line under normal conditions". Use a detail message
> > > > if needed to keep the primary message short, or if you feel a need to
> > > > mention implementation details such as the particular system call that
> > > > failed. Both primary and detail messages should be factual. Use a hint
> > > > message for suggestions about what to do to fix the problem, especially
> > > > if the suggestion might not always be applicable. It's quite common
> > > > that you'll only see the actual error message in logs and displayed
> > > > error messages.
> >
> > Having to be the owner of a relation to TRUNCATE it was an
> > implementation detail, and one which changed back in 2008.
> >
> > Needing to have the replication role attribute is an implementation
> > detail. It's not the error- the error is *permission denied to do X*.
>
> That's just plain absurd. If the user needs to explicitly configure that
> permission it's damned sure not just a implementation detail.

The implementation detail is that it's not part of the normal
GRANT/REVOKE privilege system, which is why it's useful to note it in
the detail and why we don't need to add an errdetail along the lines of
'You must have SELECT rights on relation X to SELECT from it'.

Thanks,

Stephen

On Wed, Nov 26, 2014 at 10:12 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> In the context at hand, I think most of the messages in question are
>> currently phrased like "must be superuser to do X". I'd be fine with
>> changing that to "permission denied to do X", but not to just
>> "permission denied".
>
> Apologies for the terseness of my (earlier) reply. This is exactly what
> I'm suggesting. What was in the patch was this change:
>
> ! ERROR: must be superuser or replication role to use replication slots
>
> ---
>
> ! ERROR: permission denied to use replication slots
> ! HINT: You must be superuser or replication role to use replication slots.

Your proposed change takes two lines to convey the same amount of
information that we are currently conveying in one line. How is that
better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Nov 26, 2014 at 10:12 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> In the context at hand, I think most of the messages in question are
> >> currently phrased like "must be superuser to do X". I'd be fine with
> >> changing that to "permission denied to do X", but not to just
> >> "permission denied".
> >
> > Apologies for the terseness of my (earlier) reply. This is exactly what
> > I'm suggesting. What was in the patch was this change:
> >
> > ! ERROR: must be superuser or replication role to use replication slots
> >
> > ---
> >
> > ! ERROR: permission denied to use replication slots
> > ! HINT: You must be superuser or replication role to use replication slots.
>
> Your proposed change takes two lines to convey the same amount of
> information that we are currently conveying in one line. How is that
> better?

It includes the actual error message, unlike what we have today which is
guidence as to what's required to get past the permission denied error.

In other words, I disagree that the same amount of information is being
conveyed.

Thanks,

Stephen

On Tue, Dec 2, 2014 at 11:29 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Wed, Nov 26, 2014 at 10:12 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> >> In the context at hand, I think most of the messages in question are
>> >> currently phrased like "must be superuser to do X". I'd be fine with
>> >> changing that to "permission denied to do X", but not to just
>> >> "permission denied".
>> >
>> > Apologies for the terseness of my (earlier) reply. This is exactly what
>> > I'm suggesting. What was in the patch was this change:
>> >
>> > ! ERROR: must be superuser or replication role to use replication slots
>> >
>> > ---
>> >
>> > ! ERROR: permission denied to use replication slots
>> > ! HINT: You must be superuser or replication role to use replication slots.
>>
>> Your proposed change takes two lines to convey the same amount of
>> information that we are currently conveying in one line. How is that
>> better?
>
> It includes the actual error message, unlike what we have today which is
> guidence as to what's required to get past the permission denied error.
>
> In other words, I disagree that the same amount of information is being
> conveyed.

So, you think that someone might see the message "must be superuser or
replication role to use replication slots" and fail to understand that
they have a permissions problem?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Dec 2, 2014 at 11:29 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > It includes the actual error message, unlike what we have today which is
> > guidence as to what's required to get past the permission denied error.
> >
> > In other words, I disagree that the same amount of information is being
> > conveyed.
>
> So, you think that someone might see the message "must be superuser or
> replication role to use replication slots" and fail to understand that
> they have a permissions problem?

I think that the error message about a permissions problem should be
that there is a permissions problem, first and foremost.

We don't say 'You must have SELECT rights on relation X to select from
it.' when you try to do a SELECT that you're not allowed to. We throw a
permissions denied error. As pointed out up-thread, we do similar
things for other cases of permissions denied and even beyond permission
denied errors we tend to match up the errmsg() with the errcode(), which
makes a great deal of sense to me and is why I'm advocating that
position.

Thanks,

Stephen

On Tue, Dec 2, 2014 at 2:35 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Tue, Dec 2, 2014 at 11:29 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > It includes the actual error message, unlike what we have today which is
>> > guidence as to what's required to get past the permission denied error.
>> >
>> > In other words, I disagree that the same amount of information is being
>> > conveyed.
>>
>> So, you think that someone might see the message "must be superuser or
>> replication role to use replication slots" and fail to understand that
>> they have a permissions problem?
>
> I think that the error message about a permissions problem should be
> that there is a permissions problem, first and foremost.

I can't disagree with that, but it's not like the current message is:

ERROR: I ran into some kind of a problem but I'm not going to tell you
what it is for a long time OK I guess that's enough well actually you
see there is a problem and it happens to do have to do with
permissions and in particular that they are denied.

It's pretty clear that the current message is complaining about a
permissions problem, so the fact that it doesn't specifically include
the words "permission" and "denied" doesn't bother me. Let me ask the
question again: what information do you think is conveyed by your
proposed rewrite that is not conveyed by the current message?

> We don't say 'You must have SELECT rights on relation X to select from
> it.' when you try to do a SELECT that you're not allowed to. We throw a
> permissions denied error. As pointed out up-thread, we do similar
> things for other cases of permissions denied and even beyond permission
> denied errors we tend to match up the errmsg() with the errcode(), which
> makes a great deal of sense to me and is why I'm advocating that
> position.

I don't thing that trying to match up the errmsg() with the errcode()
is a useful exercise. That's just restricting the ways that people
can write error messages in a way that is otherwise unnecessary. The
errmsg() and errcode() have different purposes; the former is to
provide a concise, human-readable description of the problem, and the
latter is to group errors into categories so that related errors can
be handled programmatically. They need not be, and in many existing
cases are not, even vaguely similar; and the fact that many of our
permission denied errors happen to use that phrasing is not a
sufficient justification for changing the rest unless the new phrasing
is a bona fide improvement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Dec 2, 2014 at 2:35 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> On Tue, Dec 2, 2014 at 11:29 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> > It includes the actual error message, unlike what we have today which is
> >> > guidence as to what's required to get past the permission denied error.
> >> >
> >> > In other words, I disagree that the same amount of information is being
> >> > conveyed.
> >>
> >> So, you think that someone might see the message "must be superuser or
> >> replication role to use replication slots" and fail to understand that
> >> they have a permissions problem?
> >
> > I think that the error message about a permissions problem should be
> > that there is a permissions problem, first and foremost.
>
> I can't disagree with that, but it's not like the current message is:
>
> ERROR: I ran into some kind of a problem but I'm not going to tell you
> what it is for a long time OK I guess that's enough well actually you
> see there is a problem and it happens to do have to do with
> permissions and in particular that they are denied.

I agree that doing this would be rather overkill.

> It's pretty clear that the current message is complaining about a
> permissions problem, so the fact that it doesn't specifically include
> the words "permission" and "denied" doesn't bother me. Let me ask the
> question again: what information do you think is conveyed by your
> proposed rewrite that is not conveyed by the current message?

I do like that it's explicitly stating that the problem is with
permissions and not because of some lack-of-capability in the software
and I like the consistency of messaging (to the point where I was
suggesting somewhere along the way that we update the error messaging
documentation to be explicit that the errmsg and the errcode should make
sense and match up- NOT that we should use some simple mapping from one
to the other as we'd then be reducing the information provided, but I've
seen a number of cases where people have the SQL error-code also
returned in their psql session; I've never been a fan of that as I much
prefer words over error-codes, but I wonder if they've done that
because, in some cases, it *isn't* clear what the errcode is from the
errmsg..).

> > We don't say 'You must have SELECT rights on relation X to select from
> > it.' when you try to do a SELECT that you're not allowed to. We throw a
> > permissions denied error. As pointed out up-thread, we do similar
> > things for other cases of permissions denied and even beyond permission
> > denied errors we tend to match up the errmsg() with the errcode(), which
> > makes a great deal of sense to me and is why I'm advocating that
> > position.
>
> I don't thing that trying to match up the errmsg() with the errcode()
> is a useful exercise. That's just restricting the ways that people
> can write error messages in a way that is otherwise unnecessary. The
> errmsg() and errcode() have different purposes; the former is to
> provide a concise, human-readable description of the problem, and the
> latter is to group errors into categories so that related errors can
> be handled programmatically. They need not be, and in many existing
> cases are not, even vaguely similar; and the fact that many of our
> permission denied errors happen to use that phrasing is not a
> sufficient justification for changing the rest unless the new phrasing
> is a bona fide improvement.

It provides a consistency of messaging that I feel is an improvement.
That they aren't related in a number of existing cases strikes me as an
opportunity to improve rather than cases where we really "know better".
Perhaps in some of those cases the problem is that there isn't a good
errcode, and maybe we should actually add an error code instead of
changing the message. Perhaps there are cases where we just know better
or it's a very generic errcode without any real meaning. I agree that
we want the messaging to be good, I'd like it to also be consistent with
itself and with the errcode.

I've not gone and tried to do a deep look at the errmsg vs errcode, and
that's likely too much to bite off at once anyway, but I have gone and
looked at the role attribute uses and the other permissions denied
errcode cases and we're not at all consistent today and we change from
release-to-release depending on changes to the permissions system (see:
TRUNCATE). I specifically don't like that.

If we want to say that role-attribute-related error messages should be
"You need X to do Y", then I'll go make those changes, removing the
'permission denied' where those are used and be happier that we're at
least consistent, but it'll be annoying if we ever make those
role-attributes be GRANT'able rights (GRANT .. ON CLUSTER?) as those
errmsg's will then change from "you need X" to "permission denied to do
Y". Having the errdetail change bothers me a lot less.

How would you feel about changing the various usages of
have_createrole_privilege() call sites to say "You must have CREATEROLE
to create role" instead of "permission denied to create role"? Today,
some of them say one, other times the other, and, for my part at least,
I don't see any particular justification for why it's one way sometimes
and the other other times, which is an example of the inconsistency I'm
complaining about. At least if we had a policy we would have hope of
getting consistency and we would still have the detail in the
errdetail(). Making these cases two lines instead of one doesn't bother
me one bit, at least.

Thanks,

Stephen

On 11/21/14 10:28 AM, Stephen Frost wrote:
> Specifically:
> ---------------
> One curious item to note is that the
> current if(!superuser()) {} block approach has masked an inconsistency
> in at least the FDW code which required a change to the regression
> tests- namely, we normally force FDW owners to have USAGE rights on
> the FDW, but that was being bypassed when a superuser makes the call.
> I seriously doubt that was intentional. I won't push back if someone
> really wants it to stay that way though.
> ---------------

I think there are some inconsistencies here.

To rephrase the problem:

1. To run CREATE SERVER, you need USAGE rights on FDW.

2. To run ALTER SERVER OWNER as unprivileged user, the new owner also
needs USAGE rights on FDW.

3. When you run ALTER SERVER OWNER as superuser, the new owner does not
need USAGE rights on FDW.

-> The proposal is to change #3 to require the new owner to have USAGE
privileges even when running as superuser.

Let's look at an analogous case:

1. To run CREATE DOMAIN, you need USAGE rights on the underlying type.

2. To run ALTER DOMAIN OWNER, the receiving user does not need USAGE
rights on the underlying type.

These two cases are inconsistent.

There might be more analogous cases involving other object classes.

Solution, either:

A. We allow unprivileged users to "give away" objects to other
unprivileged users who do not have the privileges that they could have
created the object themselves. This could be a marginally useful
feature. But it might also violate the privilege system, because it
effectively allows you to bypass the grant options.

or

B. For unprivileged users to "give away" an object to another
unprivileged user, the receiving user must have the privileges so that
they could have created the object themselves.

B.i. Superuser can bypass that restriction.

B.ii. Superusers cannot bypass that restriction.

Which one shall it be?

On Tue, Dec 2, 2014 at 4:11 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> It's pretty clear that the current message is complaining about a
>> permissions problem, so the fact that it doesn't specifically include
>> the words "permission" and "denied" doesn't bother me. Let me ask the
>> question again: what information do you think is conveyed by your
>> proposed rewrite that is not conveyed by the current message?
>
> I do like that it's explicitly stating that the problem is with
> permissions and not because of some lack-of-capability in the software
> and I like the consistency of messaging (to the point where I was
> suggesting somewhere along the way that we update the error messaging
> documentation to be explicit that the errmsg and the errcode should make
> sense and match up- NOT that we should use some simple mapping from one
> to the other as we'd then be reducing the information provided, but I've
> seen a number of cases where people have the SQL error-code also
> returned in their psql session; I've never been a fan of that as I much
> prefer words over error-codes, but I wonder if they've done that
> because, in some cases, it *isn't* clear what the errcode is from the
> errmsg..).

I think you're dodging my question. The answer is that there really
*isn't* any additional information conveyed by your proposal rewrite;
the issue is simply that you prefer your version to the current
version. Which is fair enough, but my preference is different (as is
that of Andres), which is also fair.

> It provides a consistency of messaging that I feel is an improvement.
> That they aren't related in a number of existing cases strikes me as an
> opportunity to improve rather than cases where we really "know better".

Let's consider this case, which perhaps you haven't examined:

if (es.buffers && !es.analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option BUFFERS
requires ANALYZE")));

This is basically analogous to the case your complaining about. The
error message is short and to the point and does not include the words
that appear in the error code. Now, you could propose to change this,
but any rewording of it is going to be longer than what we have now
for no real improvement in clarity, and it's going to force
translators to do the extra work of retranslating it for no particular
gain.

The burden for changing existing messages is not high, but your desire
for a different style is not sufficient to go change half the error
messages in the backend, or even 20% of them, and I think that 20% is
a conservative estimate of how many we'd have to change to actually
achieve what you're talking about here.

> Perhaps in some of those cases the problem is that there isn't a good
> errcode, and maybe we should actually add an error code instead of
> changing the message.

The error codes are mostly taken from the SQL standard. It is of
course possible that we should add our own in some cases, but it will
have the disadvantage of reducing the ability of applications written
for other systems to understand our error codes.

> If we want to say that role-attribute-related error messages should be
> "You need X to do Y", then I'll go make those changes, removing the
> 'permission denied' where those are used and be happier that we're at
> least consistent, but it'll be annoying if we ever make those
> role-attributes be GRANT'able rights (GRANT .. ON CLUSTER?) as those
> errmsg's will then change from "you need X" to "permission denied to do
> Y". Having the errdetail change bothers me a lot less.

You can't really put "you need X to do Y" in the error message, I
think. That style is appropriate for an error detail, but not an
error message.

I just don't understand why you want to pointlessly tinker with this.
If this is integrally related to the introduction of finer-grained
superuser permissions, then perhaps it is worth doing, but if that's
the motivation, you haven't made an argument that I can understand as
to why it's necessary. What I think is that the current messages - or
at least the ones you are complaining about - are fine, and we can
change them on a case-by-case basis as the evolution of the system
demands, but that we shouldn't go whack them around just because you
would have chosen differently from among sane alternatives than what
the original author chose to do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 11/21/14 10:28 AM, Stephen Frost wrote:
> > Specifically:
> > ---------------
> > One curious item to note is that the
> > current if(!superuser()) {} block approach has masked an inconsistency
> > in at least the FDW code which required a change to the regression
> > tests- namely, we normally force FDW owners to have USAGE rights on
> > the FDW, but that was being bypassed when a superuser makes the call.
> > I seriously doubt that was intentional. I won't push back if someone
> > really wants it to stay that way though.
> > ---------------
>
> I think there are some inconsistencies here.

Agreed.. We certainly have a number of inconsistencies and I'd love to
reduce them. :)

> To rephrase the problem:
>
> 1. To run CREATE SERVER, you need USAGE rights on FDW.
>
> 2. To run ALTER SERVER OWNER as unprivileged user, the new owner also
> needs USAGE rights on FDW.
>
> 3. When you run ALTER SERVER OWNER as superuser, the new owner does not
> need USAGE rights on FDW.
>
> -> The proposal is to change #3 to require the new owner to have USAGE
> privileges even when running as superuser.

On reflection, this seemed odd because of how the code was written but
perhaps it was intentional after all. In general, superuser should be
able to bypass permissions restrictions and I don't see why this case
should be different.

> Let's look at an analogous case:
>
> 1. To run CREATE DOMAIN, you need USAGE rights on the underlying type.
>
> 2. To run ALTER DOMAIN OWNER, the receiving user does not need USAGE
> rights on the underlying type.
>
> These two cases are inconsistent.
>
> There might be more analogous cases involving other object classes.
>
> Solution, either:
>
> A. We allow unprivileged users to "give away" objects to other
> unprivileged users who do not have the privileges that they could have
> created the object themselves. This could be a marginally useful
> feature. But it might also violate the privilege system, because it
> effectively allows you to bypass the grant options.
>
> or
>
> B. For unprivileged users to "give away" an object to another
> unprivileged user, the receiving user must have the privileges so that
> they could have created the object themselves.

In general, I don't think we want to allow "giving away" of objects by
unprivileged users. We don't allow that to be done for tables and I'm
surprised to hear that it's possible to give domains away.

> B.i. Superuser can bypass that restriction.
>
> B.ii. Superusers cannot bypass that restriction.

Superuser should be able to bypass the restriction, BUT the object given
away by the superuser to an unprivileged user should NOT be able to be
further given away by that unprivileged user.

Thanks!

Stephen

On 11/26/14 10:24 AM, Stephen Frost wrote:
> The implementation detail is that it's not part of the normal
> GRANT/REVOKE privilege system, which is why it's useful to note it in
> the detail and why we don't need to add an errdetail along the lines of
> 'You must have SELECT rights on relation X to SELECT from it'.

I don't agree with this argument, but I might agree with the conclusion. ;-)

I think in the past, error messages for permission problems were
effectively written according to the criterion:

"If I can explain the reason for the lack of permission in one short
line, then I will, otherwise I will just produce a generic 'permission
denied' error and have the user read the manual for the details."

The proposed change is effectively:

"I will produce a generic 'permission denied' error, and if the reason
for the lack of permission is anything other than GRANT/REVOKE, then I
will add it to the detail message."

That's not necessarily an invalid change, but it implies that there is
something special (or less special) about GRANT/REVOKE, and there is no
consensus on that.

Seeing that we are planning to add more permissions systems of various
kinds, I don't think it would be bad to uniformly add "You must have
SELECT rights on relation X to SELECT from it" detail messages. The
proposed changes would then be subset of that.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Dec 2, 2014 at 4:11 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> It's pretty clear that the current message is complaining about a
> >> permissions problem, so the fact that it doesn't specifically include
> >> the words "permission" and "denied" doesn't bother me. Let me ask the
> >> question again: what information do you think is conveyed by your
> >> proposed rewrite that is not conveyed by the current message?
> >
> > I do like that it's explicitly stating that the problem is with
> > permissions and not because of some lack-of-capability in the software
> > and I like the consistency of messaging (to the point where I was
> > suggesting somewhere along the way that we update the error messaging
> > documentation to be explicit that the errmsg and the errcode should make
> > sense and match up- NOT that we should use some simple mapping from one
> > to the other as we'd then be reducing the information provided, but I've
> > seen a number of cases where people have the SQL error-code also
> > returned in their psql session; I've never been a fan of that as I much
> > prefer words over error-codes, but I wonder if they've done that
> > because, in some cases, it *isn't* clear what the errcode is from the
> > errmsg..).
>
> I think you're dodging my question.

My answer included "not because of some lack-of-capability" which was
intended to answer your question of "what information do you think is
conveyed by your proposed rewrite that is not converyed by the current
message?"

I have a hard time wrapping my head around what a *lot* of our users ask
when they see a given error message, but if our error message is 'you
must have a pear-shaped object to run this command' then I imagine that
a new-to-PG user might think "well, I can't create pear shaped objects
in PG, so I guess this just isn't supported." That's not necessairly
any fault of ours, but I do think "permission denied" reduces the chance
that there's any confusion about the situation.

> The answer is that there really
> *isn't* any additional information conveyed by your proposal rewrite;

To be sure it's clear, I *don't* agree with this. You and I don't see
any additional information in it because we're familiar with the system
and know all about role attributes, the GRANT system, and everything
else. I'm not looking to change the error message because it's going to
make it clearer to you or to me or to anyone else on this list though.

> the issue is simply that you prefer your version to the current
> version. Which is fair enough, but my preference is different (as is
> that of Andres), which is also fair.

I do prefer my version but it's not an unfounded preference.

> > It provides a consistency of messaging that I feel is an improvement.
> > That they aren't related in a number of existing cases strikes me as an
> > opportunity to improve rather than cases where we really "know better".
>
> Let's consider this case, which perhaps you haven't examined:
>
> if (es.buffers && !es.analyze)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("EXPLAIN option BUFFERS
> requires ANALYZE")));
>
> This is basically analogous to the case your complaining about. The
> error message is short and to the point and does not include the words
> that appear in the error code. Now, you could propose to change this,
> but any rewording of it is going to be longer than what we have now
> for no real improvement in clarity, and it's going to force
> translators to do the extra work of retranslating it for no particular
> gain.

I'm on the fence about if this is really the same case. In this case,
changing 'option' to 'parameter' would make it pretty darn close to the
error code. I could also see making it longer and more explicit-
'invalid parameter (BUFFERS) for EXPLAIN' with an errdetail of what's
above.

> The burden for changing existing messages is not high, but your desire
> for a different style is not sufficient to go change half the error
> messages in the backend, or even 20% of them, and I think that 20% is
> a conservative estimate of how many we'd have to change to actually
> achieve what you're talking about here.

The "different style" is what's in the error style guidelines. I agree
that it could lead to a lot of changes and I don't think we'd need to
change them all in one shot or in one release, in part because of the
burden it would put on translators.

> > Perhaps in some of those cases the problem is that there isn't a good
> > errcode, and maybe we should actually add an error code instead of
> > changing the message.
>
> The error codes are mostly taken from the SQL standard. It is of
> course possible that we should add our own in some cases, but it will
> have the disadvantage of reducing the ability of applications written
> for other systems to understand our error codes.

This, in particular, doesn't bother me terribly much- if there's reason
enough for a new code then it's probably because it's something PG
specific enough to justify it.

> > If we want to say that role-attribute-related error messages should be
> > "You need X to do Y", then I'll go make those changes, removing the
> > 'permission denied' where those are used and be happier that we're at
> > least consistent, but it'll be annoying if we ever make those
> > role-attributes be GRANT'able rights (GRANT .. ON CLUSTER?) as those
> > errmsg's will then change from "you need X" to "permission denied to do
> > Y". Having the errdetail change bothers me a lot less.
>
> You can't really put "you need X to do Y" in the error message, I
> think. That style is appropriate for an error detail, but not an
> error message.

I'm utterly confused by this comment. The existing error messages that
I want to change are of the 'you need X to do Y' style (eg: "must be
superuser or have the same role to cancel queries running in other
server processes"). If you're just referring to the 'you' word, then
ok, I agree that it goes against the error style guidelines and it would
really be "need X to do Y", but that wasn't what I was trying to get at
with the above comment. I was saying *let's make it consistent* and go
change the existing cases which say "permission denied to create role"
and similar to, instead, say "must have CREATEROLE to create roles".
Today, we're utterly inconsistent.

> I just don't understand why you want to pointlessly tinker with this.

Because I don't feel it's pointless to improve the consistency of the
error messaging and I don't like that it's inconsistent today.

> If this is integrally related to the introduction of finer-grained
> superuser permissions, then perhaps it is worth doing, but if that's
> the motivation, you haven't made an argument that I can understand as
> to why it's necessary.

It's got nothing to do with the finer-grained superuser permissions.
This particular patch isn't even directly related to it- it's just my
feeling that these changes are in the right direction which we should be
going in and that it's an improvement in its own right to use
has_privs_of_role() instead of GetUserId() in these cases.

> What I think is that the current messages - or
> at least the ones you are complaining about - are fine, and we can
> change them on a case-by-case basis as the evolution of the system
> demands, but that we shouldn't go whack them around just because you
> would have chosen differently from among sane alternatives than what
> the original author chose to do.

I was trying to get to a more consistent system with these changes. If
no one feels that's worthwhile, then so be it. I appreciate the enegy
you've spent in responding even if we weren't ultimately able to agree
on the outcome.

Thanks,

Stephen

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> "I will produce a generic 'permission denied' error, and if the reason
> for the lack of permission is anything other than GRANT/REVOKE, then I
> will add it to the detail message."

That's what I had been thinking, on the assumption that individuals with
SQL-spec-type systems would be familiar with the GRANT/REVOKE system,
but..

> Seeing that we are planning to add more permissions systems of various
> kinds, I don't think it would be bad to uniformly add "You must have
> SELECT rights on relation X to SELECT from it" detail messages. The
> proposed changes would then be subset of that.

I'd be fine with that. It would mean an extra line of output in many
cases but we could at least be consistent across the backend with regard
to how these cases are handled...

Thanks!

Stephen

On 2014-12-04 15:59:17 -0500, Stephen Frost wrote:
> I have a hard time wrapping my head around what a *lot* of our users ask
> when they see a given error message, but if our error message is 'you
> must have a pear-shaped object to run this command' then I imagine that
> a new-to-PG user might think "well, I can't create pear shaped objects
> in PG, so I guess this just isn't supported." That's not necessairly
> any fault of ours, but I do think "permission denied" reduces the chance
> that there's any confusion about the situation.

I've a hard time taking this comment seriously. If can't believe that
you think that comment bears relation to the error message we're
discussing.

> > The answer is that there really
> > *isn't* any additional information conveyed by your proposal rewrite;
>
> To be sure it's clear, I *don't* agree with this. You and I don't see
> any additional information in it because we're familiar with the system
> and know all about role attributes, the GRANT system, and everything
> else. I'm not looking to change the error message because it's going to
> make it clearer to you or to me or to anyone else on this list though.

> The "different style" is what's in the error style guidelines.

I think you're vastly over-interpreting the guidelines because that
happens to suite your position.

None of the current error message violates any of:

> The primary message should be short, factual, and avoid reference to
> implementation details such as specific function names. "Short" means
> "should fit on one line under normal conditions". Use a detail message
> if needed to keep the primary message short, or if you feel a need to
> mention implementation details such as the particular system call that
> failed. Both primary and detail messages should be factual. Use a hint
> message for suggestions about what to do to fix the problem, especially
> if the suggestion might not always be applicable.

And I don't for a second buy your argument that the permissions involved
are an implemementation detail.

If you say that you like the new message better because it's more
consistent or more beautiful I can buy that. But don't try to find
justifications in guidelines when they don't contain that.

> > I just don't understand why you want to pointlessly tinker with this.
>
> Because I don't feel it's pointless to improve the consistency of the
> error messaging and I don't like that it's inconsistent today.

Then please do so outside of patches/threads that do something pretty
much unrelated.

Greetings,

Andres Freund

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-12-04 15:59:17 -0500, Stephen Frost wrote:
> > I have a hard time wrapping my head around what a *lot* of our users ask
> > when they see a given error message, but if our error message is 'you
> > must have a pear-shaped object to run this command' then I imagine that
> > a new-to-PG user might think "well, I can't create pear shaped objects
> > in PG, so I guess this just isn't supported." That's not necessairly
> > any fault of ours, but I do think "permission denied" reduces the chance
> > that there's any confusion about the situation.
>
> I've a hard time taking this comment seriously. If can't believe that
> you think that comment bears relation to the error message we're
> discussing.

It's reasonable to think that new users won't understand PG-specific
things such as role attributes. A new user might not recognize or
understand what "replication role" is but they're more than likely to
get the gist behind "permission denied."

> > > The answer is that there really
> > > *isn't* any additional information conveyed by your proposal rewrite;
> >
> > To be sure it's clear, I *don't* agree with this. You and I don't see
> > any additional information in it because we're familiar with the system
> > and know all about role attributes, the GRANT system, and everything
> > else. I'm not looking to change the error message because it's going to
> > make it clearer to you or to me or to anyone else on this list though.
>
> > The "different style" is what's in the error style guidelines.
>
> I think you're vastly over-interpreting the guidelines because that
> happens to suite your position.

So I shouldn't consider what the guidelines have because they agree with
my position? Perhaps we should *change* the guidelines if they aren't
what we should be following.

> None of the current error message violates any of:
>
> > The primary message should be short, factual, and avoid reference to
> > implementation details such as specific function names. "Short" means
> > "should fit on one line under normal conditions". Use a detail message
> > if needed to keep the primary message short, or if you feel a need to
> > mention implementation details such as the particular system call that
> > failed. Both primary and detail messages should be factual. Use a hint
> > message for suggestions about what to do to fix the problem, especially
> > if the suggestion might not always be applicable.

I agree that the text doesn't say "the error message should be related
to what the errcode used is", but it's certainly the implication of the
examples provided and, when the text doesn't make any reference, going
by what is in the example seems perfectly reasonable to me.

> And I don't for a second buy your argument that the permissions involved
> are an implemementation detail.

Would you agree with Peter that, rather than focus on if the errdetail()
involves an implementation detail or not, we should go ahead and add the
"You must have SELECT rights ..." to the existing cases where we just
say "permission denied"?

> If you say that you like the new message better because it's more
> consistent or more beautiful I can buy that. But don't try to find
> justifications in guidelines when they don't contain that.

I've said that I like the new messaging because it's more consistent
time and time again. You suggest that I argue on that merit alone
rather than pointing out where we've (intentionally or not) used
examples which agree with me? That strikes me as rather silly.

> > > I just don't understand why you want to pointlessly tinker with this.
> >
> > Because I don't feel it's pointless to improve the consistency of the
> > error messaging and I don't like that it's inconsistent today.
>
> Then please do so outside of patches/threads that do something pretty
> much unrelated.

Alright, I bothered to go back and find the relevant message- it's this
one: 20141022231834(dot)GA1587(at)alvin(dot)alvh(dot)no-ip(dot)org where Alvaro pointed out
that the messaging is inconsistent and asked that this patch should
include the correct wording (whatever we decide it to be) and further
commentary in 20141023232302(dot)GH1791(at)alvin(dot)alvh(dot)no-ip(dot)org matches that..
I didn't bring this up and I'm getting a bit put-out by the constant
implications that it's just all me and my crazy ideas for consistency.
Perhaps Alvaro doesn't care to argue the position further, which is
fine, but at least acknowledge that there are others who agree that
we're currently inconsistent (Alvaro and, I believe, Peter).

Thanks,

Stephen

Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > On 2014-12-04 15:59:17 -0500, Stephen Frost wrote:

> > > > I just don't understand why you want to pointlessly tinker with this.
> > >
> > > Because I don't feel it's pointless to improve the consistency of the
> > > error messaging and I don't like that it's inconsistent today.
> >
> > Then please do so outside of patches/threads that do something pretty
> > much unrelated.
>
> Alright, I bothered to go back and find the relevant message- it's this
> one: 20141022231834(dot)GA1587(at)alvin(dot)alvh(dot)no-ip(dot)org where Alvaro pointed out
> that the messaging is inconsistent and asked that this patch should
> include the correct wording (whatever we decide it to be) and further
> commentary in 20141023232302(dot)GH1791(at)alvin(dot)alvh(dot)no-ip(dot)org matches that..
> I didn't bring this up and I'm getting a bit put-out by the constant
> implications that it's just all me and my crazy ideas for consistency.
> Perhaps Alvaro doesn't care to argue the position further, which is
> fine, but at least acknowledge that there are others who agree that
> we're currently inconsistent (Alvaro and, I believe, Peter).

Several dozens messages ago in this thread I would have dropped this
item, TBH. *Maybe* I would consider changing the specific messages
around the specific code that's being tinkered with, but probably not
other ones.

I haven't looked at this patch recently, but if it's changing 1% of the
error messages in the backend, I doubt it's a good idea.

More in general, if I see strong, well-rationalized opposition to some
non-essential idea of mine, I tend to drop it because I don't see the
value in arguing endlessly. Life's already way too short.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2014-12-04 17:06:02 -0500, Stephen Frost wrote:
> * Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> > On 2014-12-04 15:59:17 -0500, Stephen Frost wrote:
> > > I have a hard time wrapping my head around what a *lot* of our users ask
> > > when they see a given error message, but if our error message is 'you
> > > must have a pear-shaped object to run this command' then I imagine that
> > > a new-to-PG user might think "well, I can't create pear shaped objects
> > > in PG, so I guess this just isn't supported." That's not necessairly
> > > any fault of ours, but I do think "permission denied" reduces the chance
> > > that there's any confusion about the situation.
> >
> > I've a hard time taking this comment seriously. If can't believe that
> > you think that comment bears relation to the error message we're
> > discussing.
>
> It's reasonable to think that new users won't understand PG-specific
> things such as role attributes. A new user might not recognize or
> understand what "replication role" is but they're more than likely to
> get the gist behind "permission denied."

Anyone that's using any of these facilities better know at least halfway
what a permission is.

> > > > The answer is that there really
> > > > *isn't* any additional information conveyed by your proposal rewrite;
> > >
> > > To be sure it's clear, I *don't* agree with this. You and I don't see
> > > any additional information in it because we're familiar with the system
> > > and know all about role attributes, the GRANT system, and everything
> > > else. I'm not looking to change the error message because it's going to
> > > make it clearer to you or to me or to anyone else on this list though.
> >
> > > The "different style" is what's in the error style guidelines.
> >
> > I think you're vastly over-interpreting the guidelines because that
> > happens to suite your position.
>
> So I shouldn't consider what the guidelines have because they agree with
> my position?

Not if there's not actually anything in there.

> Perhaps we should *change* the guidelines if they aren't what we
> should be following.

So, now you've build the full circle.

> > None of the current error message violates any of:
> >
> > > The primary message should be short, factual, and avoid reference to
> > > implementation details such as specific function names. "Short" means
> > > "should fit on one line under normal conditions". Use a detail message
> > > if needed to keep the primary message short, or if you feel a need to
> > > mention implementation details such as the particular system call that
> > > failed. Both primary and detail messages should be factual. Use a hint
> > > message for suggestions about what to do to fix the problem, especially
> > > if the suggestion might not always be applicable.
>
> I agree that the text doesn't say "the error message should be related
> to what the errcode used is", but it's certainly the implication of the
> examples provided and, when the text doesn't make any reference, going
> by what is in the example seems perfectly reasonable to me.

If anything it's the absolute contrary.

Messages should always state the reason why an error occurred. For
example:

> BAD: could not open file %s
> BETTER: could not open file %s (I/O failure)

> Avoid mentioning called function names, either; instead say what the
> code was trying to do:
> BAD: open() failed: %m
> BETTER: could not open file %s: %m

> BAD: unknown node type
> BETTER: unrecognized node type: 42

There's simply not a a single example giving credence to your position
in the guidelines I'm looking at. That's *not* to say that your position
is wrong, but holding up the guidelines as a reason for it is absurd.

> > And I don't for a second buy your argument that the permissions involved
> > are an implemementation detail.
>
> Would you agree with Peter that, rather than focus on if the errdetail()
> involves an implementation detail or not, we should go ahead and add the
> "You must have SELECT rights ..." to the existing cases where we just
> say "permission denied"?

I think adding helpful information isn't a bad idea. It's not always
obvious which permission is required to do something.

> > If you say that you like the new message better because it's more
> > consistent or more beautiful I can buy that. But don't try to find
> > justifications in guidelines when they don't contain that.
>
> I've said that I like the new messaging because it's more consistent
> time and time again. You suggest that I argue on that merit alone
> rather than pointing out where we've (intentionally or not) used
> examples which agree with me? That strikes me as rather silly.

Since there's nothing in the guidelines...

> I didn't bring this up and I'm getting a bit put-out by the constant
> implications that it's just all me and my crazy ideas for consistency.

I don't mind much being outvoted or convinced by better reasoning. But
you've shown so far no recognition of the fact that the new message is
much longer without much of additional detail. And you've largely
argumented using arguments that I found absolutely unconvincing.

I'm now giving in by plonking this thread.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-12-04 17:06:02 -0500, Stephen Frost wrote:
> > Would you agree with Peter that, rather than focus on if the errdetail()
> > involves an implementation detail or not, we should go ahead and add the
> > "You must have SELECT rights ..." to the existing cases where we just
> > say "permission denied"?
>
> I think adding helpful information isn't a bad idea. It's not always
> obvious which permission is required to do something.
[...]
> > I didn't bring this up and I'm getting a bit put-out by the constant
> > implications that it's just all me and my crazy ideas for consistency.
>
> I don't mind much being outvoted or convinced by better reasoning. But
> you've shown so far no recognition of the fact that the new message is
> much longer without much of additional detail. And you've largely
> argumented using arguments that I found absolutely unconvincing.

I agree that the new message is longer. I don't much care, no.

I'm confused by the comment above and then this one- surely error
messages from lack of SELECT or similar rights are way more commonly
emitted than ones about the replication role attribute? You are,
seemingly, agreeing with making the error message emitted when SELECT is
used longer while saying that we shouldn't make the error message about
the replication role attribute longer?

Is there something special about the replciation role attribute case
that makes it different from the SELECT rights?

Thanks,

Stephen

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> Several dozens messages ago in this thread I would have dropped this
> item, TBH.

I'm getting to that point, though it's quite frustrating. I didn't much
care initially but it's gotten to the point where the current situation
just strikes me as quite 'wrong'.

> *Maybe* I would consider changing the specific messages
> around the specific code that's being tinkered with, but probably not
> other ones.

If I thought that was a way to move forward, then I'd be happy with it.
I have no problem with this being a policy of "make it match the policy
when you change it, but don't just go changing everything right away as
it'll cause too much disruption." I'm mostly argueing about what the
policy should be here and less about the specific minor changes in this
patch.

I'm pretty sure that's not acceptable to Robert or Andres though, as I
think they're also argueing policy. I can certainly understand a
position which is "it's not wrong and we shouldn't change it for the
sake of changing it."

> I haven't looked at this patch recently, but if it's changing 1% of the
> error messages in the backend, I doubt it's a good idea.

This just confuses me- you said above that you'd consider changing just
the specific messages around the specific code being tinkered with
(which would be less than 1%..) and then say it's not a good idea to
change just 1%? The actual patch is a mostly unrelated (and,
seemingly, not terribly controversial) change, as compared to this
discussion about error messages.

> More in general, if I see strong, well-rationalized opposition to some
> non-essential idea of mine, I tend to drop it because I don't see the
> value in arguing endlessly. Life's already way too short.

I'm not sure that I see it as well-rationalized, but it certainly seems
to be consistent and the error message changes are certainly
non-essential. Perhaps it'd be better discussed in Ottawa.

Thanks,

Stephen

On Thu, Dec 4, 2014 at 3:59 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I have a hard time wrapping my head around what a *lot* of our users ask
> when they see a given error message, but if our error message is 'you
> must have a pear-shaped object to run this command' then I imagine that
> a new-to-PG user might think "well, I can't create pear shaped objects
> in PG, so I guess this just isn't supported." That's not necessairly
> any fault of ours, but I do think "permission denied" reduces the chance
> that there's any confusion about the situation.

This is just ridiculous. You're postulating the existing of a person
who thinks that a replication role is something that they buy at
Wendi's rather than something granted by the database administrator.
Give me a break.

> I do prefer my version but it's not an unfounded preference.

I never said that your preference was unfounded. I said that I
disagreed with it.

>> The burden for changing existing messages is not high, but your desire
>> for a different style is not sufficient to go change half the error
>> messages in the backend, or even 20% of them, and I think that 20% is
>> a conservative estimate of how many we'd have to change to actually
>> achieve what you're talking about here.
>
> The "different style" is what's in the error style guidelines. I agree
> that it could lead to a lot of changes and I don't think we'd need to
> change them all in one shot or in one release, in part because of the
> burden it would put on translators.

As Andres says, that's just plain not true. There is nothing
whatsoever in the error guidelines that supports your position on this
issue.

Reasoning logically, you're saying that this could "could lead to a
lot of changes". You're also claiming that it is supported by the
message style guidelines. Taken together, these two statements imply
that you think that a lot of our current messages are not in
conformance with the message style guidelines. But how did all of
those non-comformant messages get there, and how is it that no one has
ever felt the urge to fix it up until now? After all, it is not as if
message style is a topic that never gets discussed here; it does,
quite regularly. A more reasonable explanation is that your
interpretation of the message style guidelines disagrees with that of
other people on this list.

> This, in particular, doesn't bother me terribly much- if there's reason
> enough for a new code then it's probably because it's something PG
> specific enough to justify it.

Fair enough. I don't know enough about the merits or demerits of
inventing new error codes to have a clear feeling for whether it would
be a good idea or not. But that's not the ostensible topic of this
thread.

>> > If we want to say that role-attribute-related error messages should be
>> > "You need X to do Y", then I'll go make those changes, removing the
>> > 'permission denied' where those are used and be happier that we're at
>> > least consistent, but it'll be annoying if we ever make those
>> > role-attributes be GRANT'able rights (GRANT .. ON CLUSTER?) as those
>> > errmsg's will then change from "you need X" to "permission denied to do
>> > Y". Having the errdetail change bothers me a lot less.
>>
>> You can't really put "you need X to do Y" in the error message, I
>> think. That style is appropriate for an error detail, but not an
>> error message.
>
> I'm utterly confused by this comment. The existing error messages that
> I want to change are of the 'you need X to do Y' style (eg: "must be
> superuser or have the same role to cancel queries running in other
> server processes"). If you're just referring to the 'you' word, then
> ok, I agree that it goes against the error style guidelines and it would
> really be "need X to do Y", but that wasn't what I was trying to get at
> with the above comment.

Yes, I was talking about the "you".

> I was saying *let's make it consistent* and go
> change the existing cases which say "permission denied to create role"
> and similar to, instead, say "must have CREATEROLE to create roles".
> Today, we're utterly inconsistent.

Consistency is a virtue, but not the only one or the highest one. I
think that when the rule is something simple, it makes sense to
articulate it in the error message, but when the rule gets too
complicated to be articulated in brief, then we must give a generic
permission denied message and expect the user to go read the manual
for more information. Suppose we consider two hypothetical
operations, fire-united-states-nukes and punish-recalcitrant-child.
The first one is a highly restricted operation, but the criteria are
simple enough that they can be stated succinctly:

ERROR: must be POTUS to launch US nukes

The second operation is far less restricted in terms of who can carry
it out, but whether it's allowable in a particular instance is
complicated, depending as it does on your relationship to the child in
question and the proposed punishment. Parents can probably safely
assume they can send their child to their room; and teachers their
students to detention; but other punishments may depend on
circumstance, and then there are other roles like babysitter,
principals, guidance counselor, and substantially-older sibling that
each have rules all of their own. It will be impractical to concisely
state what will pass muster, so we'd better go with something like:

ERROR: permission denied to punish recalcitrant child

This is not because it wouldn't be *nice* to give a more specific
error message saying exactly what criteria you failed to meet; it's
just not really practical to generate an error message for that. But
that doesn't mean that we're better off changing the first message to
say:

ERROR: permission denied to launch US nukes

IMHO, that's just pointlessly leaving the user hanging about just how
important they have to be when we could have told them the exact rule
in fewer characters than it took to be less specific.

Now, I realize that you may not agree with this philosophy, but I
think that *is* the philosophy behind the current lack of consistency.
Counterexamples welcome, of course: if there are inconsistencies that
are NOT explained by the principle I just articulated, I might well
support revising those cases.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Dec 4, 2014 at 3:59 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I have a hard time wrapping my head around what a *lot* of our users ask
> > when they see a given error message, but if our error message is 'you
> > must have a pear-shaped object to run this command' then I imagine that
> > a new-to-PG user might think "well, I can't create pear shaped objects
> > in PG, so I guess this just isn't supported." That's not necessairly
> > any fault of ours, but I do think "permission denied" reduces the chance
> > that there's any confusion about the situation.
>
> This is just ridiculous. You're postulating the existing of a person
> who thinks that a replication role is something that they buy at
> Wendi's rather than something granted by the database administrator.
> Give me a break.

I was pointing out that it might not be recognizable as a permission by
a person new to PG, yes. I did not characterize that individual as a
database administrator, nor would they characterize themselves that way
(at least, the person I'm thinking of).

> > I do prefer my version but it's not an unfounded preference.
>
> I never said that your preference was unfounded. I said that I
> disagreed with it.

And this doesn't seem like it's going to change and certainly not over
email and so I'm going to go with Alvaro's approach and simply drop it.
I'll finish up the changes that I commented on above and move on.

Thanks,

Stephen

On 12/4/14 3:32 PM, Stephen Frost wrote:
> On reflection, this seemed odd because of how the code was written but
> perhaps it was intentional after all. In general, superuser should be
> able to bypass permissions restrictions and I don't see why this case
> should be different.

> In general, I don't think we want to allow "giving away" of objects by
> unprivileged users. We don't allow that to be done for tables and I'm
> surprised to hear that it's possible to give domains away.

> Superuser should be able to bypass the restriction, BUT the object given
> away by the superuser to an unprivileged user should NOT be able to be
> further given away by that unprivileged user.

Clearly, this issue is a bit more complex than a simple code cleanup.
So I'm going to set this patch as returned with feedback.

My suggestion for moving forward would be to define a general security
policy for the ALTER OWNER cases, and then fix those properly.

The changes for integration the superuser check into the replication
role check should perhaps be tackled as part of a general refactoring of
capability checks.