On Fri, Aug 03, 2018 at 02:08:27PM +0100, Alexandra Ryzhevich wrote: > Thank you for pointing to some problems of the patch. I've attached a > modified version below.
Could you avoid top-posting? This is not the style of the Postgres mailing lists. I have been looking at your latest patch, and there are some gotchas: - There is no need for the initial DROP ROLE commands as those already get dropped at the end of the tests. - Some installations may use non-default settings, causing installcheck to fail with your patch once stats_temp_directory is using a different path than pg_stat_tmp. - The same applies for archive_timeout. - There is already rolenames.sql which has a tiny coverage for default roles, why not just using it? +-- should fail because regress_role_nopriv has not CONNECT permission on this db +SELECT pg_database_size('regression') > 0 AS canread; +ERROR: permission denied for database regression +-- should fail because regress_role_nopriv has not CREATE permission on this tablespace +SELECT pg_tablespace_size('pg_global') > 0 AS canread; +ERROR: permission denied for tablespace pg_global Why is that part of a test suite for default roles? There are three code paths which could trigger the restrictions we are looking at for pg_read_all_settings: - GetConfigOptionResetString, which is only used for datetyle now. - GetConfigOptionByName, which can be triggered with any SHOW command. - GetConfigOption, which is used at postmaster startup and when reloading the configuration file. 2) is easy to be triggered as a negative test (which fails), less as a positive test. In order to make a positive test failure-proof with installcheck you would need to have a parameter which can be changed by a superuser at session level which gets updated to a certain value, and would fail to show for another user, so you could use one which is GUC_SUPERUSER_ONLY and of category PGC_SUSET, like session_preload_libraries or dynamic_preload_libraries. Still that's pretty restrictive, and would only test one out of the three code paths available. 1) and 3) have restrictions in place visibly mainly for module developers. -- Michael
signature.asc
Description: PGP signature