Обсуждение: [PATCH] Add regress test for pg_read_all_stats role

On Thu, Aug 02, 2018 at 06:25:14PM +0100, Alexandra Ryzhevich wrote:
> I have noticed that there is no regression tests for default monitoring
> roles such as pg_read_all_stats.

A bug has been recently fixed for that, see 0c8910a0, so having more
coverage would be welcome, now your patch has a couple of problems.
25fff40 is the original commit which has introduced pg_read_all_stats.

Your patch has a couple of problems by the way:
- database creation is costly, those should not be part of the main
regression test suite.
- Any roles created should use "regress_" as prefix.
- You should look also at negative tests which trigger "must be
superuser or a member of pg_read_all_settings", like say a "SHOW
stats_temp_directory" with a non-privileged user.
--
Michael

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

On Tue, Aug 21, 2018 at 05:48:49PM +0100, Alexandra Ryzhevich wrote:
> Just to check if changes broke something. I haven't find these checks in
> other regress tests. In other way we get only positive tests. If this
> is not needed then should I remove all the checks for
> regress_role_nopriv role or negative regress_role_nopriv tests only?

+-- should not fail because regress_role_haspriv is a member of
pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
What is really disturbing about the ones testing the size functions is
that they may be costly when running installcheck.  By the way, it would
be nice to avoid the system-wide REVOKE, which could impact any tests
running in parallel.  You could simply check for some other NULL
values.

> Changed to use session_preload_libraries.

+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO '/tmp/';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;

This gets better, thanks.  I would suggest using a less realistic value
than "/tmp/", which could become a security hole if copy-pasted around...
--
Michael

On Fri, Aug 24, 2018 at 03:37:17PM +0100, Alexandra Ryzhevich wrote:
> CONNECT ON DATABASE privilege is granted to public by default (so
> all users by default can select pg_database_size()) and
> REVOKE CONNECT ... FROM <username> command doesn't impact
> user's privileges. The only way to revoke connect privilege from user
> is to revoke it from public. That's why maybe it will be less harmful
> just to remove pg_database_size tests at all. But will it be better to
> remove pg_tablespace_size() tests also? if possible costs are more
> harmful than not testing this code path then I'll remove these tests
> also.

Sorry for the delay, Alexandra, and thanks for the new version.  I have
removed the tests related to pg_tablespace_size as they are costly and
those for vacuum_cost_delay as we didn't gain more with those tests.
The rest has been committed, which makes a good first cut.
--
Michael