From 9715143492fef2c1f430f3149820420ad83a0c61 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 7 Nov 2026 15:00:00 -0500 Subject: [PATCH] fix: a restore with pg_restore 17 into Postgres 16 isn't stopped by a setting the server lacks pg_restore 17 opens with `SET transaction_timeout = 0`, which Postgres 16 refuses. Under --exit-on-error that one harmless statement ended every restore: the image ships pg_restore 17.11 and the bundled server is pg16, so a restore from the bucket (and the local restore) could never have loaded a dump. The backup round trip, now that CI has pg_dump, found it. pg_restore now runs to the end and its errors are judged afterwards: the known harmless one is ignored, and any other fails the restore with that error's words rather than the "Command was:" line that followed it. Also sorts an import the lint lane flagged. Co-Authored-By: Claude Opus 5.5 --- backend/app/services/backup_service.py | 51 +++++++++++++++++++++----- tests/test_backup_restore.py | 2 +- tests/test_backup_service.py | 43 +++++++++++++++++++++- 3 files changed, 85 insertions(+), 11 deletions(-) diff --git a/backend/app/services/backup_service.py b/backend/app/services/backup_service.py index 6164f4f..ca4dabf 100644 --- a/backend/app/services/backup_service.py +++ b/backend/app/services/backup_service.py @@ -347,17 +347,50 @@ def wipe_db(*, db_url: str) -> None: ) +# A pg_restore newer than the server sets parameters the server doesn't have +# (pg_restore 17 against Postgres 16: `SET transaction_timeout = 0`). The +# statement fails and nothing depends on it, so its error is not a failed +# restore. +_HARMLESS_RESTORE_ERRORS = ( + 'unrecognized configuration parameter "transaction_timeout"', +) + + +def _restore_errors(stderr: str) -> list[str]: + """pg_restore's error lines, less the harmless ones.""" + return [ + line.strip() for line in stderr.splitlines() + if "error:" in line and not any(h in line for h in _HARMLESS_RESTORE_ERRORS) + ] + + def restore_db(*, db_url: str, sql_path: Path) -> None: - """Wipe the public schema, then load the custom-format dump. pg_restore - stops at the first error, which RestoreFailed carries; the database is - then partly loaded, and the caller decides what to put back.""" + """Wipe the public schema, then load the custom-format dump. Any error + pg_restore reports, other than a harmless one, raises RestoreFailed with + the first; the database is then partly loaded, and the caller decides what + to put back.""" wipe_db(db_url=db_url) - # Custom-format (-Fc) dumps are restored with pg_restore, not psql. - _run_reporting( - _gently(["pg_restore", "--no-owner", "--no-acl", "--exit-on-error", - "-d", _libpq_url(db_url), str(sql_path)]), - _RESTORE_SUBPROCESS_TIMEOUT_S, - ) + # Custom-format (-Fc) dumps are restored with pg_restore, not psql. It runs + # on past errors (it can't skip the harmless one and stop at the rest), so + # its errors are judged once it is done. + cmd = _gently(["pg_restore", "--no-owner", "--no-acl", + "-d", _libpq_url(db_url), str(sql_path)]) + with tempfile.TemporaryFile() as err: + proc = subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=err, + timeout=_RESTORE_SUBPROCESS_TIMEOUT_S) + if proc.returncode == 0: + return + err.seek(0) + said = err.read().decode(errors="replace") + errors = _restore_errors(said) + if errors: + raise RestoreFailed(errors[0] if len(errors) == 1 + else f"{errors[0]} ({len(errors)} errors in all)") + if not any(h in said for h in _HARMLESS_RESTORE_ERRORS): + # A failure with no error line to show for it. + lines = said.strip().splitlines() + raise RestoreFailed(lines[-1].strip() if lines + else f"pg_restore exited with {proc.returncode}") def restore_images(*, images_root: Path, tar_path: Path) -> None: diff --git a/tests/test_backup_restore.py b/tests/test_backup_restore.py index 6968aaf..614f47d 100644 --- a/tests/test_backup_restore.py +++ b/tests/test_backup_restore.py @@ -198,7 +198,7 @@ def test_a_restore_out_of_time_queues_its_next_chunk(db_sync, tmp_path, run_env, def test_a_restart_resumes_the_file_phase_and_drops_the_stale_chunk( db_sync, tmp_path, run_env, ): - from backend.app.tasks.backup import resume_offsite_runs, restore_offsite_task + from backend.app.tasks.backup import restore_offsite_task, resume_offsite_runs _file(tmp_path, "a.jpg", b"aa") _image(db_sync, tmp_path / "a.jpg", b"aa") now = datetime.now(UTC) diff --git a/tests/test_backup_service.py b/tests/test_backup_service.py index 33046a2..32c2e15 100644 --- a/tests/test_backup_service.py +++ b/tests/test_backup_service.py @@ -173,11 +173,52 @@ def test_restore_db_drops_schema_then_loads(tmp_path, fake_subprocess): assert "-c" in fake_subprocess[0] assert "DROP SCHEMA IF EXISTS public CASCADE" in fake_subprocess[0][-1] assert "pg_terminate_backend" in fake_subprocess[0][-1] - assert "pg_restore" in fake_subprocess[1] and "--exit-on-error" in fake_subprocess[1] + assert "pg_restore" in fake_subprocess[1] assert "-d" in fake_subprocess[1] assert str(dump_path) in fake_subprocess[1] +def _pg_restore_says(monkeypatch, stderr_text: bytes): + class _Done: + returncode = 1 + + class _Ok: + returncode = 0 + + def run(cmd, stderr=None, **kw): + if "pg_restore" in cmd: + stderr.write(stderr_text) + return _Done() + return _Ok() + + monkeypatch.setattr("subprocess.run", run) + + +_NEWER_CLIENT = ( + b'pg_restore: error: could not execute query: ERROR: unrecognized configuration ' + b'parameter "transaction_timeout"\nCommand was: SET transaction_timeout = 0;\n' +) + + +def test_a_newer_pg_restore_setting_what_the_server_lacks_is_not_a_failure( + tmp_path, monkeypatch, +): + _pg_restore_says(monkeypatch, _NEWER_CLIENT + + b"pg_restore: warning: errors ignored on restore: 1\n") + backup_service.restore_db(db_url="postgresql://u@h/d", sql_path=tmp_path / "x.dump") + + +def test_a_real_error_beside_the_harmless_one_fails_the_restore_with_its_words( + tmp_path, monkeypatch, +): + _pg_restore_says(monkeypatch, _NEWER_CLIENT + + b'pg_restore: error: could not execute query: ERROR: relation ' + b'"tag" does not exist\nCommand was: COPY tag FROM stdin;\n' + + b"pg_restore: warning: errors ignored on restore: 2\n") + with pytest.raises(backup_service.RestoreFailed, match='relation "tag" does not exist'): + backup_service.restore_db(db_url="postgresql://u@h/d", sql_path=tmp_path / "x.dump") + + def test_a_failed_restore_says_what_pg_restore_said(tmp_path, monkeypatch): class _Done: def __init__(self, code):