woblerr commented on issue #90: URL: https://github.com/apache/cloudberry-backup/issues/90#issuecomment-4295270168
Hi, @my-ship-it Thanks for the detailed feedback and thoughtful design considerations. > **1. Raw rsync of a live SQLite file isn't safe.** SQLite's own docs advise against file-level copies of an open database (WAL/journal state can produce a torn snapshot on the destination). Even though `DoCleanup` calls `historyDB.Close()` before the proposed sync point, a concurrent `gpbackup` invocation can still be mid-write. Two safer options: > > * Export a consistent snapshot first (`VACUUM INTO '/tmp/gpbackup_history.db.snap'` or the `sqlite3_backup_*` online-backup API), then rsync the snapshot. > > * Or hold the existing history lockfile for the duration of the rsync. > > > I'd lean toward the snapshot approach — it keeps the sync window short and doesn't block other backups. Look's like `VACUUM INTO '/tmp/gpbackup_history.db.snap'` is the right choice. It can be done from Go (via the mattn/go-sqlite3 driver) so we don't depend on an external sqlite3 binary. > **2. Sync timing — just at the end, or also at start?** The current proposal only syncs after the final status update. If the process is killed hard (OOM, host crash) before `DoCleanup` runs, the "In Progress" row never reaches standby, and post-failover the backup looks like it never happened. Adding a sync right after the initial insert — debounced or rate-limited if you're worried about overhead — would close that window. I think we should only sync at the end. Syncing before the backup adds no real protection (standby should already have the previous history). If the backup itself fails, losing the new history entry is not critical — it will be re-synced on the next successful run. In addition, we need to think about sync history db after maintenance operations from gpbackman. For example, if we delete backups, we should sync the history db to reflect that change. But that is a separate issue and can be done after implementing the basic sync functionality. > **3. Standby detection.** Querying `gp_segment_configuration WHERE content = -1 AND role = 'm' AND status = 'u'` is probably cleaner than shelling out to `gpstate`. If the standby is marked down, we should skip silently to stay consistent with the "non-fatal" principle. Agreed, we should detect standby presence via SQL query from `gp_segment_configuration`. > **4. Destination-side atomicity.** On the standby, write to a temp file first and `rename(2)` into place, so a concurrent reader on the standby (post-promotion, for example) never observes a half-written DB. Agreed, the main logic is: * Primary: `VACUUM INTO` a timestamped file (e.g. `gpbackup_history_<ts>.db.snap`). * Transfer: rsync that snapshot into the standby’s directory where gpbackup_history.db lives. * Standby: execute `gpbackup_helper --install-history-snapshot --snapshot-path '<snap>' --dest-path '<dest>' --owner '<user:group>'`; the helper opens the snapshot, runs `PRAGMA quick_check;` (or it could be `PRAGMA integrity_check;`, but it is slower), sets perms/owner, and atomically renames the snapshot into place. Errors are logged; installation failures do not break the backup run. > **5. Scope — just the DB?** The `gpbackup__report` files in MDD are also useful for post-failover troubleshooting and have the same single-coordinator problem. Worth deciding up front whether this feature covers them too, or we leave that for a follow-up. Agreed, it’s possible/valuable, but this is a bigger design question: gpbackman changes would need a plan for syncing report files and handling deletions. That should be scoped as a dedicated issue so we can design the semantics safely. > **6. Flag name.** `--no-sync-standby` reads (to me) as if it's about syncing backup data to the standby in general. Something like `--no-history-sync-standby` or `--no-standby-history-sync` would be more specific. Minor nit, very open to your preference. Thanks, `--no-history-sync-standby` — sounds good. That complements the existing `--no-history` flag. > **7. SSH assumptions.** Passwordless SSH from coordinator to standby is standard in Cloudberry clusters, so assuming it is fine — but containerized/locked-down deployments will fail, which is exactly why your non-fatal-with-warning default is the right call. That depends on deployment, but the project already uses rsync+ssh and it’s the simplest/most compatible choice. Alternate transports can be considered later if needed. If in feature we add alternative formats for storing backup history information (e.g. a shared filesystem or separate PG https://github.com/apache/cloudberry-backup/issues/80#issuecomment-4176061414), this functionality is still be useful for clusters that use the file-based approach. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
