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
signature.asc
Description: Digital signature
