Browse Source

Use shell redirection rather than the --file flag to sidestep synchronization issues when pg_dump/pg_dumpall tries to write to a named pipe.

Dan Helfman 5 years ago
parent
commit
6a61070d85
2 changed files with 23 additions and 27 deletions
  1. 6 3
      borgmatic/hooks/postgresql.py
  2. 17 24
      tests/unit/hooks/test_postgresql.py

+ 6 - 3
borgmatic/hooks/postgresql.py

@@ -42,15 +42,16 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
                 '--no-password',
                 '--clean',
                 '--if-exists',
-                '--no-sync',
             )
-            + ('--file', dump_filename)
             + (('--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 ())
             + (() if all_databases else ('--format', database.get('format', 'custom')))
             + (tuple(database['options'].split(' ')) if 'options' in database else ())
             + (() if all_databases else (name,))
+            # Use shell redirection rather than the --file flag to sidestep synchronization issues
+            # when pg_dump/pg_dumpall tries to write to a named pipe.
+            + ('>', dump_filename)
         )
         extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None
 
@@ -65,7 +66,9 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
         dump.create_named_pipe_for_dump(dump_filename)
 
         processes.append(
-            execute_command(command, extra_environment=extra_environment, run_to_completion=False)
+            execute_command(
+                command, shell=True, extra_environment=extra_environment, run_to_completion=False
+            )
         )
 
     return processes

+ 17 - 24
tests/unit/hooks/test_postgresql.py

@@ -20,13 +20,13 @@ def test_dump_databases_runs_pg_dump_for_each_database():
                 '--no-password',
                 '--clean',
                 '--if-exists',
-                '--no-sync',
-                '--file',
-                'databases/localhost/{}'.format(name),
                 '--format',
                 'custom',
                 name,
+                '>',
+                'databases/localhost/{}'.format(name),
             ),
+            shell=True,
             extra_environment=None,
             run_to_completion=False,
         ).and_return(process).once()
@@ -61,9 +61,6 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port():
             '--no-password',
             '--clean',
             '--if-exists',
-            '--no-sync',
-            '--file',
-            'databases/database.example.org/foo',
             '--host',
             'database.example.org',
             '--port',
@@ -71,7 +68,10 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port():
             '--format',
             'custom',
             'foo',
+            '>',
+            'databases/database.example.org/foo',
         ),
+        shell=True,
         extra_environment=None,
         run_to_completion=False,
     ).and_return(process).once()
@@ -94,15 +94,15 @@ def test_dump_databases_runs_pg_dump_with_username_and_password():
             '--no-password',
             '--clean',
             '--if-exists',
-            '--no-sync',
-            '--file',
-            'databases/localhost/foo',
             '--username',
             'postgres',
             '--format',
             'custom',
             'foo',
+            '>',
+            'databases/localhost/foo',
         ),
+        shell=True,
         extra_environment={'PGPASSWORD': 'trustsome1'},
         run_to_completion=False,
     ).and_return(process).once()
@@ -125,13 +125,13 @@ def test_dump_databases_runs_pg_dump_with_format():
             '--no-password',
             '--clean',
             '--if-exists',
-            '--no-sync',
-            '--file',
-            'databases/localhost/foo',
             '--format',
             'tar',
             'foo',
+            '>',
+            'databases/localhost/foo',
         ),
+        shell=True,
         extra_environment=None,
         run_to_completion=False,
     ).and_return(process).once()
@@ -154,14 +154,14 @@ def test_dump_databases_runs_pg_dump_with_options():
             '--no-password',
             '--clean',
             '--if-exists',
-            '--no-sync',
-            '--file',
-            'databases/localhost/foo',
             '--format',
             'custom',
             '--stuff=such',
             'foo',
+            '>',
+            'databases/localhost/foo',
         ),
+        shell=True,
         extra_environment=None,
         run_to_completion=False,
     ).and_return(process).once()
@@ -179,15 +179,8 @@ def test_dump_databases_runs_pg_dumpall_for_all_databases():
     flexmock(module.dump).should_receive('create_named_pipe_for_dump')
 
     flexmock(module).should_receive('execute_command').with_args(
-        (
-            'pg_dumpall',
-            '--no-password',
-            '--clean',
-            '--if-exists',
-            '--no-sync',
-            '--file',
-            'databases/localhost/all',
-        ),
+        ('pg_dumpall', '--no-password', '--clean', '--if-exists', '>', 'databases/localhost/all'),
+        shell=True,
         extra_environment=None,
         run_to_completion=False,
     ).and_return(process).once()