Thread: pgsql: Switch TAP tests of pg_rewind to use a role with minimalpermiss

Switch TAP tests of pg_rewind to use a role with minimal permissions

Up to now the tests of pg_rewind have been using a superuser for all the
tests (which is the default of many tests actually, and something that
ought to be reviewed) when involving an online source server, still it
is possible to use a non-superuser role to do that as long as this role
is granted permissions to execute all the source-side functions used for
the rewind.  This is possible since v11, and was already documented as
of bfc8068.

This will allow to catch up easily any change in pg_rewind if the tool
begins to use more backend-side functions, so as the properties
introduced by v11 are kept.

Per suggestion from Peter Eisentraut.

Author: Michael Paquier
Reviewed-by: Magnus Hagander
Discussion: https://postgr.es/m/20190411041336.GM2728@paquier.xyz

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/d4e2a843e6d6f325c070ee80a0c117ec11675e74

Modified Files
--------------
src/bin/pg_rewind/t/RewindTest.pm | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

On 4/11/19 9:56 PM, Michael Paquier wrote:
> Switch TAP tests of pg_rewind to use a role with minimal permissions
>
> Up to now the tests of pg_rewind have been using a superuser for all the
> tests (which is the default of many tests actually, and something that
> ought to be reviewed) when involving an online source server, still it
> is possible to use a non-superuser role to do that as long as this role
> is granted permissions to execute all the source-side functions used for
> the rewind.  This is possible since v11, and was already documented as
> of bfc8068.
>
> This will allow to catch up easily any change in pg_rewind if the tool
> begins to use more backend-side functions, so as the properties
> introduced by v11 are kept.
>


jacana and bowerbird don't seem happy with this.


cheers


andrew


-- 

Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> jacana and bowerbird don't seem happy with this.

I thought that might be an aberration, but a quick review suggests that
none of the other Windows buildfarm members are running the pg_rewind
test.

It's fairly hard to see how we get from this patch to those failures,
though ...

            regards, tom lane

On April 13, 2019 11:59:23 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> jacana and bowerbird don't seem happy with this.
>
> I thought that might be an aberration, but a quick review suggests that
> none of the other Windows buildfarm members are running the pg_rewind
> test.
>
> It's fairly hard to see how we get from this patch to those failures,
> though ...

If you look at the logs, SSPI authentication fails because there is no user mapping for the user added by this commit
inthe tests. So the answer is that the configuration files need to be updated so as the configuration is able to work.
Idon't have time to work on that before Monday using my windows environment so I think that for now it is better to
revertthe thing. I'll try do to that asap. 

--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On April 13, 2019 11:59:23 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's fairly hard to see how we get from this patch to those failures,
>> though ...

> If you look at the logs, SSPI authentication fails because there is no
> user mapping for the user added by this commit in the tests. So the
> answer is that the configuration files need to be updated so as the
> configuration is able to work. I don't have time to work on that before
> Monday using my windows environment so I think that for now it is better
> to revert the thing. I'll try do to that asap.

Hm.  Seems like that test case could also use some work so that it's
more obvious when it has connection problems.

            regards, tom lane

On Fri, Apr 12, 2019 at 11:54:48PM -0400, Tom Lane wrote:
> Hm.  Seems like that test case could also use some work so that it's
> more obvious when it has connection problems.

For the problem reported, the deal is that we need to add in
pg_indent.conf a mapping to the new user created (as done in
config_sspi_auth()).  We can do that with pg_regress --create-role and
PostgresNode.pm::init() can help with that.  So the fix is simple
enough.  I'll make sure that it works properly at the beginning of
next week.  For now the patch is reverted.
--
Michael

On 2019-Apr-13, Michael Paquier wrote:

> On Fri, Apr 12, 2019 at 11:54:48PM -0400, Tom Lane wrote:
> > Hm.  Seems like that test case could also use some work so that it's
> > more obvious when it has connection problems.
> 
> For the problem reported, the deal is that we need to add in
> pg_indent.conf a mapping to the new user created (as done in
> config_sspi_auth()).  We can do that with pg_regress --create-role and
> PostgresNode.pm::init() can help with that.  So the fix is simple
> enough.  I'll make sure that it works properly at the beginning of
> next week.  For now the patch is reverted.

I'm not sure that reverting and re-committing a slightly improved patch
is a better approach than just waiting for the (presumably small) fix to
be committed.  Leaving a small number of animals in red for a small
period is not that bad.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Apr 13, 2019 at 11:12:25AM -0400, Alvaro Herrera wrote:
> I'm not sure that reverting and re-committing a slightly improved patch
> is a better approach than just waiting for the (presumably small) fix to
> be committed.  Leaving a small number of animals in red for a small
> period is not that bad.

Thanks, however I am not so sure when it comes to Windows failures as
these can prove to need tricky treatments.  Anyway, there is a daily
commit activity and it is always annoying to bump on failures from
others and this could hide other problems.  947a350 and 9daefff are
for example two things from Noah which refer to jacana and have een
committed in-between, so reverting temporarily was still the best
thing to do I think.

I have committed the fixed version of this patch, after making sure
that the test is able to work on Windows, and that this does not
impact what we want to test: if one of the GRANT queries is removed
the test fails properly.
--
Michael