The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

* Applies cleanly to current master (3063e7a84026ced2aadd2262f75eebbe6240f85b)
* ./configure && make -j4 ok

DOCUMENTATION
* Documentation ok, both code (code comments) and docs.
* Documentation covers signalling backends/backup/monitor as well as the 
obvious modification to the role sections

CODE
* Checks on roles are fairly comprehensive: restrict reserved rolenames 
(creation/modification), prohibit granting to reserved roles
* The patch is surprisingly non-intrusive/self-contained considering the 
functionality.

TOOLS
* Covers pg_upgrade -- "/* 9.5 and below should not have roles starting with 
pg_ */"
* Covers pg_dumpall (do not export creation of system-reserved roles)
* Includes support in psql (\dgS) + accompanying documentation

REGRESSION TESTS
* Includes regression tests; Seem quite complete (including GRANT/REVOKE on 
reserved roles)
   Suggestion for committer: add regression tests for each reserved role? (just 
for completeness' sake)
* make installcheck-world passes; build on amd64 / gcc4.9.2 (Debian 4.9.2-10)
 - btree_gin tests fail / no contrib installed; Assumed ok
* Nitpick: tests mention the still nonexistant pg_monitor / pg_backup reserved 
roles ; Might as well use some obviously reserved-but-absurd rolename instead


Comment for future enhancement: might make sense to split role checking/access 
control functionality into a separate module, as opposed to having to include 
pg_authid.h everywhere
I'm Thinking about Michael and Heikki's upcoming authentication revamp re. 
SCRAM/multiple authenticators: authentication != authorization (apropos 
"has_privs_of_role()" )

Testing:
- pg_signal_backend Ok

Looking forward to seeing the other proposed default roles in!


The new status of this patch is: Ready for Committer

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to