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

Fixed Broken pg_dumpall Restore #34

Merged
merged 4 commits into from Apr 24, 2020
Merged

Fixed Broken pg_dumpall Restore #34

merged 4 commits into from Apr 24, 2020

Conversation

ndbeals
Copy link
Contributor

@ndbeals ndbeals commented Apr 10, 2020

When the database name is set to 'all', borgmatic uses pg_dumpall instead of pg_dump to dump the database. pg_dumpall outputs a plain-text format and pg_restore does not accept that format. To run these plain-text outputs, psql is needed instead.

This pull request implements that. Thanks!

@witten
Copy link
Collaborator

witten commented Apr 22, 2020

Thank you so much for finding and fixing this! Would you like to take a stab at updating tests for this fix? If so, here are some places to start:

  • In tests/end-to-end/test_database.py, in the test borgmatic configuration, you could add a - name: all config to the test databases. That should exercise the new code path you added for "all" database dumps/restores.
  • Some of the unit tests will need updating based on the new command arguments. They can be found in tests/unit/hooks/test_postgresql.py.

Either way, let me know!

Updated the other unit tests, as I had to re-arrange argument order
Added an 'all' test for the postgres end-to-end test.

Ran black formatter on it all.
@ndbeals
Copy link
Contributor Author

ndbeals commented Apr 24, 2020

I finished those tests, including adding another unit test. It passes all tests, including end-to-end.

There is a caveat to dumping all on a postgres database. A dump all includes all users, including the one you log into the database as to dump it. When restoring the database, even a brand-new postgres database has a user (defaults as 'postgres') which is required to run the restore all through. In short this means the user will these errors show up in their console:

ERROR:  current user cannot be dropped
STATEMENT:  DROP ROLE IF EXISTS postgres;
ERROR:  role "postgres" already exists
STATEMENT:  CREATE ROLE postgres;

But that's okay, It's failing successfully. Any further errors they get may be a problem though. It's a quirk of postgres and I don't see how it can be handled in borgmatic at all.

@witten
Copy link
Collaborator

witten commented Apr 24, 2020

Awesome.. Thanks for taking the time to add tests! And for the tip about the postgres users.. I'm glad to hear that's "just" a warning and not a blocker.

@witten witten merged commit dce1928 into borgmatic-collective:master Apr 24, 2020
@witten
Copy link
Collaborator

witten commented Apr 24, 2020

Just released in borgmatic 1.5.2! Thanks again.

@ndbeals
Copy link
Contributor Author

ndbeals commented Apr 24, 2020

Glad to have helped! you made it easy.

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