fix(backups): dump the database once instead of twice - #4916
Open
joaquinleondev wants to merge 1 commit into
Open
fix(backups): dump the database once instead of twice#4916joaquinleondev wants to merge 1 commit into
joaquinleondev wants to merge 1 commit into
Conversation
getBackupCommand ran the full dump command twice: once with its output discarded, only to check that it works, and once more piped into rclone. Every database backup therefore read and compressed the whole database two times, doubling the wall clock, the CPU cost and — for postgres — the window in which pg_dump holds ACCESS SHARE on every table. The dump is now piped straight into rclone. Each side of the pipe keeps its own stderr file so the log still distinguishes a failed dump from a failed upload; rclone is checked first because an upload that dies early kills the dump with SIGPIPE. Since the object is now written as the stream is produced, runners pass an rclone delete command that removes the truncated object when the dump fails halfway through. Closes Dokploy#4915
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4915
Problem
getBackupCommandinterpolates${backupCommand}twice: once with its stdout thrown away, purely to check that the dump works, and once more piped intorclone. Every database backup — postgres, mysql, mariadb, mongo, libsql and compose — therefore reads and compresses the entire database two times.On a 9.4 GB Postgres this is what the log looks like today:
16m31s instead of ~8m. It also doubles the CPU burned by single-threaded
gzip/pg_dump -Fcon the same box that serves traffic, and — the part that actually hurts — it doubles the window in whichpg_dumpholdsACCESS SHAREon every table and keeps a snapshot open, which is the window where a concurrent migration'sALTER TABLEblocks and autovacuum is held back.What changed
The dump is piped straight into rclone, so the database is read once and the stream is uploaded as it is produced.
Three details worth reviewing:
PIPESTATUStells which side failed, so the log keeps distinguishing❌ Error: Backup failedfrom❌ Error: Upload to S3 failed. (PIPESTATUSonly survives if the pipeline is not wrapped in$(...)— hence the temp files rather than a command substitution.) As a side effect the dump's stderr now actually reaches the log on the upload path; previously2>&1 >/dev/nullbound to the rclone side only.SIGPIPE, so a non-zero dump status is a consequence, not the cause.rclone deletecommand that removes it when the dump fails. (Worth noting the discarded first run never really prevented this: the second dump could fail just as well and leave the same truncated file.)The script still relies on bash, exactly as before (
set -o pipefailis already bash-only).One behavioural change to be aware of: the
Starting upload to S3...log line is gone, since there is no longer a separate upload phase — the two✅lines are now emitted together at the end. I checked that nothing in the codebase parses those strings.Testing
Added
apps/dokploy/__test__/backups/backup-command.test.ts, which runs the generated script for real withdocker/rclonestubs onPATH(same approach as the existingdb-backup-restore-injection.test.ts) and asserts:Backup failed, still dumping only once;Upload to S3 failed, not as a dump failure;Full suite, before and after, on the same machine (the failures are pre-existing and environmental — they need a database and a swarm manager, neither of which exists in my sandbox):
canarybiome checkclean on the touched files,tsc --noEmitclean inpackages/server.Happy to drop the cleanup-command part into a separate PR if you would rather keep this one to the single-dump change.