superuser() shortcuts

[Stephen Frost]

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

[Brightwell, Adam]

Stephen,

We, as a community, have gotten flak from time-to-time about the

Post by Stephen Frost
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.

Post by Stephen Frost
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

Post by Stephen Frost
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

[Stephen Frost]

Adam,

Post by Brightwell, Adam

Post by Stephen Frost
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.

Post by Brightwell, Adam
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.

Post by Brightwell, Adam
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?

Post by Brightwell, Adam
* 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).

Post by Brightwell, Adam
* has_replication_privilege

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

Post by Brightwell, Adam
* has_???_privilege

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

Post by Brightwell, Adam
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

[Brightwell, Adam]

Stephen,

Thanks! Please add it to the next commitfest.


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

Post by Stephen Frost
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.

Post by Stephen Frost
* has_createrole_privilege

Post by Brightwell, Adam
* has_bypassrls_privilege

These are already in the right place, right?

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

Post by Stephen Frost

Post by Brightwell, Adam
* 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.

Post by Stephen Frost

Post by Brightwell, Adam
* has_???_privilege

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

Correct.

-Adam

[Brightwell, Adam]

All,

Post by Stephen Frost
Thanks! Please add it to the next commitfest.
Sounds good. I'll update the patch and add accordingly.

Attached is an updated patch.

-Adam

[Alvaro Herrera]

Post by Brightwell, Adam
All,

Post by Stephen Frost
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

Post by Brightwell, Adam
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);
!

[Brightwell, Adam]

Alvaro,

Post by Alvaro Herrera
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

[Brightwell, Adam]

Post by Brightwell, Adam

Post by Alvaro Herrera
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?
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