From 874fba76725ce76803b586525c57c59760404593 Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 15:10:44 +0200 Subject: [PATCH 1/6] Fix PostgreSQL hook not using "psql_command" for list when dumping "all" --- borgmatic/hooks/postgresql.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index d4799f5f..c0d4619b 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -59,8 +59,9 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run): if dry_run: return () + psql_command = database.get('psql_command') or 'psql' list_command = ( - ('psql', '--list', '--no-password', '--csv', '--tuples-only') + (psql_command, '--list', '--no-password', '--csv', '--tuples-only') + (('--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 19a00371f5da3c501a8158b1dfb70ba40c09ac08 Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 15:49:49 +0200 Subject: [PATCH 2/6] Run "psql" with "--no-psqlrc" Some settings in user's .psqlrc, e.g. "linestyle unicode", may break the CSV output. "--no-psqlrc" tells psql to not read startup file. This is not necessary for the analyze_command and restore_command (with all_databases), but it's generally recommended when running psql from a script. --- borgmatic/hooks/postgresql.py | 6 +++--- tests/unit/hooks/test_postgresql.py | 18 +++++++++++------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index c0d4619b..1235d60e 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -61,7 +61,7 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run): psql_command = database.get('psql_command') or 'psql' list_command = ( - (psql_command, '--list', '--no-password', '--csv', '--tuples-only') + (psql_command, '--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only') + (('--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 ()) @@ -205,7 +205,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, ) psql_command = database.get('psql_command') or 'psql' analyze_command = ( - (psql_command, '--no-password', '--quiet') + (psql_command, '--no-password', '--no-psqlrc', '--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 ()) @@ -219,7 +219,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, + ( ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) if not all_databases - else () + else ('--no-psqlrc',) ) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index 349c04be..704d5a71 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -56,6 +56,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_hostnam 'psql', '--list', '--no-password', + '--no-psqlrc', '--csv', '--tuples-only', '--host', @@ -75,7 +76,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_hostnam def test_database_names_to_dump_with_all_and_format_lists_databases_with_username(): database = {'name': 'all', 'format': 'custom', 'username': 'postgres'} flexmock(module).should_receive('execute_command_and_capture_output').with_args( - ('psql', '--list', '--no-password', '--csv', '--tuples-only', '--username', 'postgres'), + ('psql', '--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only', '--username', 'postgres'), extra_environment=object, ).and_return('foo,test,\nbar,test,"stuff and such"') @@ -88,7 +89,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_usernam def test_database_names_to_dump_with_all_and_format_lists_databases_with_options(): database = {'name': 'all', 'format': 'custom', 'list_options': '--harder'} flexmock(module).should_receive('execute_command_and_capture_output').with_args( - ('psql', '--list', '--no-password', '--csv', '--tuples-only', '--harder'), + ('psql', '--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only', '--harder'), extra_environment=object, ).and_return('foo,test,\nbar,test,"stuff and such"') @@ -433,7 +434,7 @@ def test_restore_database_dump_runs_pg_restore(): extra_environment={'PGSSLMODE': 'disable'}, ).once() flexmock(module).should_receive('execute_command').with_args( - ('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), + ('psql', '--no-password', '--no-psqlrc', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), extra_environment={'PGSSLMODE': 'disable'}, ).once() @@ -487,6 +488,7 @@ def test_restore_database_dump_runs_pg_restore_with_hostname_and_port(): ( 'psql', '--no-password', + '--no-psqlrc', '--quiet', '--host', 'database.example.org', @@ -535,6 +537,7 @@ def test_restore_database_dump_runs_pg_restore_with_username_and_password(): ( 'psql', '--no-password', + '--no-psqlrc', '--quiet', '--username', 'postgres', @@ -580,6 +583,7 @@ def test_restore_database_dump_runs_pg_restore_with_options(): ( 'psql', '--no-password', + '--no-psqlrc', '--quiet', '--dbname', 'foo', @@ -603,14 +607,14 @@ def test_restore_database_dump_runs_psql_for_all_database_dump(): flexmock(module).should_receive('make_dump_path') flexmock(module.dump).should_receive('make_database_dump_filename') flexmock(module).should_receive('execute_command_with_processes').with_args( - ('psql', '--no-password'), + ('psql', '--no-password', '--no-psqlrc'), processes=[extract_process], output_log_level=logging.DEBUG, input_file=extract_process.stdout, extra_environment={'PGSSLMODE': 'disable'}, ).once() flexmock(module).should_receive('execute_command').with_args( - ('psql', '--no-password', '--quiet', '--command', 'ANALYZE'), + ('psql', '--no-password', '--no-psqlrc', '--quiet', '--command', 'ANALYZE'), extra_environment={'PGSSLMODE': 'disable'}, ).once() @@ -644,7 +648,7 @@ def test_restore_database_dump_runs_non_default_pg_restore_and_psql(): extra_environment={'PGSSLMODE': 'disable'}, ).once() flexmock(module).should_receive('execute_command').with_args( - ('special_psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), + ('special_psql', '--no-password', '--no-psqlrc', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), extra_environment={'PGSSLMODE': 'disable'}, ).once() @@ -689,7 +693,7 @@ def test_restore_database_dump_without_extract_process_restores_from_disk(): extra_environment={'PGSSLMODE': 'disable'}, ).once() flexmock(module).should_receive('execute_command').with_args( - ('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), + ('psql', '--no-password', '--no-psqlrc', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'), extra_environment={'PGSSLMODE': 'disable'}, ).once() From 195024e505746ce9eb096422352762619b8fd429 Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 16:11:42 +0200 Subject: [PATCH 3/6] Fix psql_command and pg_restore_command to accept command with arguments These commands are executed without `shell=True`, so the subprocess module treats e.g. "docker exec my_pg_container psql" as a single command (resulting in Errno 2 "No such file or directory") instead of a command with arguments. --- borgmatic/hooks/postgresql.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 1235d60e..fbe890a7 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -1,6 +1,7 @@ import csv import logging import os +import shlex from borgmatic.execute import ( execute_command, @@ -59,9 +60,10 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run): if dry_run: return () - psql_command = database.get('psql_command') or 'psql' + psql_command = shlex.split(database.get('psql_command') or 'psql') list_command = ( - (psql_command, '--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only') + tuple(psql_command) + + ('--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only') + (('--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 ()) @@ -203,9 +205,10 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, dump_filename = dump.make_database_dump_filename( make_dump_path(location_config), database['name'], database.get('hostname') ) - psql_command = database.get('psql_command') or 'psql' + psql_command = shlex.split(database.get('psql_command') or 'psql') analyze_command = ( - (psql_command, '--no-password', '--no-psqlrc', '--quiet') + tuple(psql_command) + + ('--no-password', '--no-psqlrc', '--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 ()) @@ -213,9 +216,10 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, + (tuple(database['analyze_options'].split(' ')) if 'analyze_options' in database else ()) + ('--command', 'ANALYZE') ) - pg_restore_command = database.get('pg_restore_command') or 'pg_restore' + pg_restore_command = shlex.split(database.get('pg_restore_command') or 'pg_restore') restore_command = ( - (psql_command if all_databases else pg_restore_command, '--no-password') + tuple(psql_command if all_databases else pg_restore_command) + + ('--no-password',) + ( ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) if not all_databases From dfccc1b94a57cf791bf4fe06b4353e20fa75c2ee Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 16:27:45 +0200 Subject: [PATCH 4/6] Exit on error when restoring all PostgreSQL databases "--set ON_ERROR_STOP=on" is equivalent to "--exit-on-error" in pg_restore. --- borgmatic/hooks/postgresql.py | 2 +- tests/unit/hooks/test_postgresql.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index fbe890a7..b91b97b8 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -223,7 +223,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, + ( ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) if not all_databases - else ('--no-psqlrc',) + else ('--no-psqlrc', '--set', 'ON_ERROR_STOP=on') ) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index 704d5a71..5f02978c 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -607,7 +607,7 @@ def test_restore_database_dump_runs_psql_for_all_database_dump(): flexmock(module).should_receive('make_dump_path') flexmock(module.dump).should_receive('make_database_dump_filename') flexmock(module).should_receive('execute_command_with_processes').with_args( - ('psql', '--no-password', '--no-psqlrc'), + ('psql', '--no-password', '--no-psqlrc', '--set', 'ON_ERROR_STOP=on'), processes=[extract_process], output_log_level=logging.DEBUG, input_file=extract_process.stdout, From f0f43174c6be21157d2d30955e14f04ddcead8a2 Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 16:29:26 +0200 Subject: [PATCH 5/6] Swap if-else in restore_database_dump in postgresql hook for cleanliness --- borgmatic/hooks/postgresql.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index b91b97b8..0f1a2df4 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -221,9 +221,9 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, tuple(psql_command if all_databases else pg_restore_command) + ('--no-password',) + ( - ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) - if not all_databases - else ('--no-psqlrc', '--set', 'ON_ERROR_STOP=on') + ('--no-psqlrc', '--set', 'ON_ERROR_STOP=on') + if all_databases + else ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) ) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) From 17f122bfe54a95e30cd186b552ae0efd295f7aa3 Mon Sep 17 00:00:00 2001 From: Jakub Jirutka Date: Fri, 14 Apr 2023 17:16:42 +0200 Subject: [PATCH 6/6] Use psql instead of pg_restore when format is "plain" pg_restore: error: input file appears to be a text format dump. Please use psql. --- borgmatic/hooks/postgresql.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 0f1a2df4..a64bb534 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -216,15 +216,17 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, + (tuple(database['analyze_options'].split(' ')) if 'analyze_options' in database else ()) + ('--command', 'ANALYZE') ) + use_psql_command = all_databases or database.get('format') == 'plain' pg_restore_command = shlex.split(database.get('pg_restore_command') or 'pg_restore') restore_command = ( - tuple(psql_command if all_databases else pg_restore_command) + tuple(psql_command if use_psql_command else pg_restore_command) + ('--no-password',) + ( ('--no-psqlrc', '--set', 'ON_ERROR_STOP=on') - if all_databases - else ('--if-exists', '--exit-on-error', '--clean', '--dbname', database['name']) + if use_psql_command + 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 ())