Re: [PATCH] Add regress test for pg_read_all_stats role

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