From b14f371c0524b4e085d0252da76240a48f11b8ae Mon Sep 17 00:00:00 2001 From: Nathan Beals Date: Fri, 10 Apr 2020 09:20:00 -0400 Subject: [PATCH 1/4] First attempt at fixing this pg_dumpall/restoring issue --- borgmatic/hooks/postgresql.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 7a46b268..889dda91 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -93,14 +93,25 @@ def restore_database_dumps(databases, log_prefix, location_config, dry_run): dump_filename = dump.make_database_dump_filename( make_dump_path(location_config), database['name'], database.get('hostname') ) - restore_command = ( - ('pg_restore', '--no-password', '--clean', '--if-exists', '--exit-on-error') - + (('--host', database['hostname']) if 'hostname' in database else ()) - + (('--port', str(database['port'])) if 'port' in database else ()) - + (('--username', database['username']) if 'username' in database else ()) - + ('--dbname', database['name']) - + (dump_filename,) - ) + if database['name'] == 'all': + restore_command = ( + ('psql', '--no-password', '--clean', '--if-exists', '--exit-on-error') + + (('--host', database['hostname']) if 'hostname' in database else ()) + + (('--port', str(database['port'])) if 'port' in database else ()) + + (('--username', database['username']) if 'username' in database else ()) + + ('--dbname', database['name']) + + (dump_filename,) + ) + else: + restore_command = ( + ('pg_restore', '--no-password', '--clean', '--if-exists', '--exit-on-error') + + (('--host', database['hostname']) if 'hostname' in database else ()) + + (('--port', str(database['port'])) if 'port' in database else ()) + + (('--username', database['username']) if 'username' in database else ()) + + ('--dbname', database['name']) + + (dump_filename,) + ) + extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None analyze_command = ( ('psql', '--no-password', '--quiet') From d5e9f67cec74e669b1dca7b1a1453ceffdf73d4f Mon Sep 17 00:00:00 2001 From: Nathan Beals Date: Fri, 10 Apr 2020 10:55:11 -0400 Subject: [PATCH 2/4] Finished. Now uses 'psql' to run the plain-text scripts that pg_dumpall creates --- borgmatic/hooks/postgresql.py | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 889dda91..5f9546a8 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -34,7 +34,7 @@ def dump_databases(databases, log_prefix, location_config, dry_run): ) all_databases = bool(name == 'all') command = ( - ('pg_dumpall' if all_databases else 'pg_dump', '--no-password', '--clean') + ('pg_dumpall' if all_databases else 'pg_dump', '--no-password', '--clean','--if-exists') + ('--file', dump_filename) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) @@ -93,34 +93,23 @@ def restore_database_dumps(databases, log_prefix, location_config, dry_run): dump_filename = dump.make_database_dump_filename( make_dump_path(location_config), database['name'], database.get('hostname') ) - if database['name'] == 'all': - restore_command = ( - ('psql', '--no-password', '--clean', '--if-exists', '--exit-on-error') - + (('--host', database['hostname']) if 'hostname' in database else ()) - + (('--port', str(database['port'])) if 'port' in database else ()) - + (('--username', database['username']) if 'username' in database else ()) - + ('--dbname', database['name']) - + (dump_filename,) - ) - else: - restore_command = ( - ('pg_restore', '--no-password', '--clean', '--if-exists', '--exit-on-error') - + (('--host', database['hostname']) if 'hostname' in database else ()) - + (('--port', str(database['port'])) if 'port' in database else ()) - + (('--username', database['username']) if 'username' in database else ()) - + ('--dbname', database['name']) - + (dump_filename,) - ) - - extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None + all_databases = bool(database['name'] == 'all') analyze_command = ( ('psql', '--no-password', '--quiet') + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--username', database['username']) if 'username' in database else ()) - + ('--dbname', database['name']) + ('--command', 'ANALYZE') ) + restore_command = ( + ('psql' if all_databases else 'pg_restore', '--no-password') + + (('--if-exists', '--exit-on-error', '--clean', '--create', '--dbname', database['name']) if not all_databases else ()) + + (('--host', database['hostname']) if 'hostname' in database else ()) + + (('--port', str(database['port'])) if 'port' in database else ()) + + (('--username', database['username']) if 'username' in database else ()) + + ( ('-f', dump_filename) if all_databases else (dump_filename,) ) + ) + extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None logger.debug( '{}: Restoring PostgreSQL database {}{}'.format( From f6407bafcb56aa3af46fa750e55bb3c3e58df466 Mon Sep 17 00:00:00 2001 From: Nathan Beals Date: Fri, 10 Apr 2020 11:20:46 -0400 Subject: [PATCH 3/4] Remove the `--create` flag, was causing an error --- borgmatic/hooks/postgresql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 5f9546a8..e308076d 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -103,7 +103,7 @@ def restore_database_dumps(databases, log_prefix, location_config, dry_run): ) restore_command = ( ('psql' if all_databases else 'pg_restore', '--no-password') - + (('--if-exists', '--exit-on-error', '--clean', '--create', '--dbname', database['name']) if not all_databases else ()) + + (('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) if not all_databases else ()) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--username', database['username']) if 'username' in database else ()) From 3c8dc4929f81b04b4dd3ccc16bef5ff32673e98f Mon Sep 17 00:00:00 2001 From: Nathan Beals Date: Fri, 24 Apr 2020 18:32:53 -0400 Subject: [PATCH 4/4] Added `test_restore_all_database_dump` unit test. 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. --- borgmatic/hooks/postgresql.py | 16 ++++++++-- tests/end-to-end/test_database.py | 4 +++ tests/unit/hooks/test_postgresql.py | 45 ++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index e308076d..9fa96330 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -34,7 +34,12 @@ def dump_databases(databases, log_prefix, location_config, dry_run): ) all_databases = bool(name == 'all') command = ( - ('pg_dumpall' if all_databases else 'pg_dump', '--no-password', '--clean','--if-exists') + ( + 'pg_dumpall' if all_databases else 'pg_dump', + '--no-password', + '--clean', + '--if-exists', + ) + ('--file', dump_filename) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) @@ -99,15 +104,20 @@ def restore_database_dumps(databases, log_prefix, location_config, dry_run): + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--username', database['username']) if 'username' in database else ()) + + (('--dbname', database['name']) if not all_databases else ()) + ('--command', 'ANALYZE') ) restore_command = ( ('psql' if all_databases else 'pg_restore', '--no-password') - + (('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) if not all_databases else ()) + + ( + ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) + if not all_databases + else () + ) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--username', database['username']) if 'username' in database else ()) - + ( ('-f', dump_filename) if all_databases else (dump_filename,) ) + + (('-f', dump_filename) if all_databases else (dump_filename,)) ) extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None diff --git a/tests/end-to-end/test_database.py b/tests/end-to-end/test_database.py index a011c6f3..53290c89 100644 --- a/tests/end-to-end/test_database.py +++ b/tests/end-to-end/test_database.py @@ -29,6 +29,10 @@ hooks: hostname: postgresql username: postgres password: test + - name: all + hostname: postgresql + username: postgres + password: test mysql_databases: - name: test hostname: mysql diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index a7c23025..31ced60d 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -17,6 +17,7 @@ def test_dump_databases_runs_pg_dump_for_each_database(): 'pg_dump', '--no-password', '--clean', + '--if-exists', '--file', 'databases/localhost/{}'.format(name), '--format', @@ -54,6 +55,7 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port(): 'pg_dump', '--no-password', '--clean', + '--if-exists', '--file', 'databases/database.example.org/foo', '--host', @@ -83,6 +85,7 @@ def test_dump_databases_runs_pg_dump_with_username_and_password(): 'pg_dump', '--no-password', '--clean', + '--if-exists', '--file', 'databases/localhost/foo', '--username', @@ -110,6 +113,7 @@ def test_dump_databases_runs_pg_dump_with_format(): 'pg_dump', '--no-password', '--clean', + '--if-exists', '--file', 'databases/localhost/foo', '--format', @@ -135,6 +139,7 @@ def test_dump_databases_runs_pg_dump_with_options(): 'pg_dump', '--no-password', '--clean', + '--if-exists', '--file', 'databases/localhost/foo', '--format', @@ -157,7 +162,14 @@ def test_dump_databases_runs_pg_dumpall_for_all_databases(): flexmock(module.os).should_receive('makedirs') flexmock(module).should_receive('execute_command').with_args( - ('pg_dumpall', '--no-password', '--clean', '--file', 'databases/localhost/all'), + ( + 'pg_dumpall', + '--no-password', + '--clean', + '--if-exists', + '--file', + 'databases/localhost/all', + ), extra_environment=None, ).once() @@ -197,9 +209,9 @@ def test_restore_database_dumps_restores_each_database(): ( 'pg_restore', '--no-password', - '--clean', '--if-exists', '--exit-on-error', + '--clean', '--dbname', name, 'databases/localhost/{}'.format(name), @@ -225,15 +237,15 @@ def test_restore_database_dumps_runs_pg_restore_with_hostname_and_port(): ( 'pg_restore', '--no-password', - '--clean', '--if-exists', '--exit-on-error', + '--clean', + '--dbname', + 'foo', '--host', 'database.example.org', '--port', '5433', - '--dbname', - 'foo', 'databases/localhost/foo', ), extra_environment=None, @@ -269,13 +281,13 @@ def test_restore_database_dumps_runs_pg_restore_with_username_and_password(): ( 'pg_restore', '--no-password', - '--clean', '--if-exists', '--exit-on-error', - '--username', - 'postgres', + '--clean', '--dbname', 'foo', + '--username', + 'postgres', 'databases/localhost/foo', ), extra_environment={'PGPASSWORD': 'trustsome1'}, @@ -296,3 +308,20 @@ def test_restore_database_dumps_runs_pg_restore_with_username_and_password(): ).once() module.restore_database_dumps(databases, 'test.yaml', {}, dry_run=False) + + +def test_restore_all_database_dump(): + databases = [{'name': 'all'}] + flexmock(module).should_receive('make_dump_path').and_return('') + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( + 'databases/localhost/all' + ) + + flexmock(module).should_receive('execute_command').with_args( + ('psql', '--no-password', '-f', 'databases/localhost/all'), extra_environment=None + ).once() + flexmock(module).should_receive('execute_command').with_args( + ('psql', '--no-password', '--quiet', '--command', 'ANALYZE'), extra_environment=None + ).once() + + module.restore_database_dumps(databases, 'test.yaml', {}, dry_run=False)