Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allows to run 'barman switch-wal --force' when the user has the 'pg_checkpoint' role #845

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

toydarian
Copy link
Contributor

Since CHECKPOINT; can be run by regular users that have the pg_checkpoint-role, we can use this to run switch-wal --force without superuser-privileges.

I implemented a method that will check if the current user has that role (or is a superuser) for PostgreSQL 14 and above and just checks for superuser-privileges for older versions of PostgreSQL.

Related issue: #844

@mikewallace1979
Copy link
Contributor

Thanks for filing #844 and making this PR.

I tested locally and this appears to work as advertised - here using PostgreSQL 15 and a non-superuser:

$ barman switch-wal main --force
ERROR: Barman switch-wal --force requires superuser rights or the 'pg_checkpoint' role

After granting the pg_checkpoint role:

$ barman switch-wal main --force
The WAL file 000000010000000100000098 has been closed on server 'main'

The patch itself looks solid however the unit tests will need updating before we can merge it - the test case tests.test_postgres.TestPostgres.test_checkpoint is currently failing because it is testing for the old PostgresSuperuserRequired exception instead of the new PostgresCheckpointPrivilegesRequired exception, and the test case will also need to set server_version on the server.postgres mock so that the code under test gets an int instead of a mock.

Ideally that test case could be expanded so that it covers both the PG < 14 and the PG >= 14 behaviour.

Are you happy to update this test case to cover the patch?

Thanks again for the contribution - it'll be great to get this merged.

@toydarian
Copy link
Contributor Author

Sorry for breaking unit-tests. I ran pytest on tests, but it seems it didn't pick up all tests, my bad.
I fixed the failing test and I wrote an additional one for the has_checkpoint_privileges property.

I also ran tox this time and it seems there are additional tests failing (in test_cli.py and test_fs.py), but I don't think those are related to my changes.

@mikewallace1979 mikewallace1979 merged commit 0596f30 into EnterpriseDB:master Sep 20, 2023
7 checks passed
@mikewallace1979
Copy link
Contributor

Sorry for breaking unit-tests. I ran pytest on tests, but it seems it didn't pick up all tests, my bad. I fixed the failing test and I wrote an additional one for the has_checkpoint_privileges property.

No worries and thank you for following up with them so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants