Re: [PATCH] Add regress test for pg_read_all_stats role

On Thu, Aug 23, 2018 at 9:12 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> 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.
>
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.

> 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...
>
Changed to unrealistic 'path-to-preload-libraries'.

Alexandra