Uh oh!
There was an error while loading. Please reload this page.
gpbackman: env-var fallback for --history-db and fail loud when DB is missing - #97
Conversation
…d when missing. Two improvements to how gpbackman locates and opens gpbackup_history.db: 1. When --history-db is empty, fall back (in priority order) to $COORDINATOR_DATA_DIRECTORY and $MASTER_DATA_DIRECTORY before the current directory. These variables are exported by the standard Cloudberry/Greenplum environment scripts, so users running gpbackman from a sourced cluster shell no longer need to repeat --history-db on every invocation. 2. OpenHistoryDB now pre-checks the file with os.Stat and opens the SQLite database with the rw URI mode. A missing file produces a friendly error pointing at --history-db and the env vars, instead of silently creating an empty file that later fails with "no such table: backups" when callers issue queries. The previous cwd-only fallback is preserved as a last resort. Tests cover the new env-var resolution order and the safe-open behaviour. Signed-off-by: chenqiang <chenqiang@hashdata.cn>
…ADME.md. Replace the "current directory" wording everywhere it appeared with the new resolution order: $COORDINATOR_DATA_DIRECTORY, $MASTER_DATA_DIRECTORY, then the current directory. Signed-off-by: chenqiang <chenqiang@hashdata.cn>
b566222 to
ef444f4CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…y-db opt-in. Address review feedback on the original PR: * The env-var fallback ($COORDINATOR_DATA_DIRECTORY then $MASTER_DATA_DIRECTORY) is now opt-in via a new persistent flag --auto-load-history-db. The default behaviour (current-directory lookup) is unchanged from upstream main, so destructive subcommands (backup-delete, backup-clean, history-clean) cannot silently target the wrong cluster's history DB on a multi-cluster jump host. * Drop the "Greenplum" mention in the comment for getHistoryDBPath to keep wording aligned with the Cloudberry environment scripts. * Trim the verbose --history-db description in gpbackman/README.md and add a separate --auto-load-history-db line. * Refine the not-found error message in OpenHistoryDB to mention the new flag rather than implying that setting env vars alone helps. * Tests updated to cover the opt-in matrix: env vars are ignored when the flag is off, honoured when on, and an explicit --history-db still wins regardless. The OpenHistoryDB safe-open change (os.Stat pre-check + mode=rw URI to avoid silently creating an empty SQLite file) is preserved as-is. Signed-off-by: chenqiang <chenqiang@hashdata.cn>
Uh oh!
There was an error while loading. Please reload this page.
…istoryDB. Previously a permission-denied or I/O failure on the history DB path returned the bare os.Stat error, which gave no hint about which operation or path failed. Wrap it with fmt.Errorf using %w so errors.Is/As still work while surfacing "stat history db <path>" in the message, matching the friendly tone of the ErrNotExist branch.
Uh oh!
There was an error while loading. Please reload this page.
@woblerr CI s3_plugin_e2e failed in the locale-setup step (dnf 404s |
Cloudberry switched from MASTER_DATA_DIRECTORY to COORDINATOR_DATA_DIRECTORY starting around release 1.5.4, and the standard environment scripts no longer export the legacy variable. Maintaining a fallback for it in gpbackman just adds surface area (constants, help text, docs, tests) for a value that should not exist in a current Cloudberry install. Drop MASTER_DATA_DIRECTORY entirely from the resolution chain, the flag help, the per-command long help strings, README/COMMANDS docs, and the corresponding test cases. The flag description is shortened in root.go to match the concise wording already used in README. The lingering "Cloudberry/Greenplum" comment in constants.go is updated to "Cloudberry" in the same pass. Resolution chain after this change: explicit --history-db -> $COORDINATOR_DATA_DIRECTORY (only when --auto-load-history-db is set) -> current working directory (default) Addresses review feedback from woblerr, tuhaihe, and MisterRaindrop on PR apache#97. The pre-existing MASTER_DATA_DIRECTORY reference in .github/workflows/cloudberry-backup-ci.yml predates this PR and is unrelated to history-db resolution; it is left for a separate cleanup.
talmacschen-arch
commented
Apr 29, 2026
@tuhaihe@woblerr — pushed 2d88689 with the MASTER_DATA_DIRECTORY cc @MisterRaindrop for visibility — feel free to re-review once CI clears. |
woblerr
commented
Apr 29, 2026
LGTM, thank you for your efforts. |
MisterRaindrop
commented
Apr 30, 2026
LGTM |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#96
Motivation
When
gpbackman backup-info(or any other gpbackman subcommand) is runwithout
--history-db, the current code drops down to a relativegpbackup_history.dbresolved against the current directory. Twoproblems follow:
The cluster's real history DB lives under
$COORDINATOR_DATA_DIRECTORY/gpbackup_history.db(or the legacy$MASTER_DATA_DIRECTORYon older installs), which is rarely theuser's shell
cwd. Operators end up typing the long path on everyinvocation despite the relevant env vars already being set by the
standard cluster environment scripts.
SQLite's default open mode silently creates a new empty database
when the file is missing. The very next query then fails with
no such table: backups, which is confusing and leaves an emptygpbackup_history.dblitter incwd.Reproducer (current behaviour on
main):Changes
gpbackman/cmd/wrappers.go—getHistoryDBPathresolves the pathin order:
--history-db→$COORDINATOR_DATA_DIRECTORY→$MASTER_DATA_DIRECTORY→ bare filename incwd(preserved forbackward compatibility).
gpbackman/gpbckpconfig/utils_db.go—OpenHistoryDBnowpre-checks the file with
os.Statand opens SQLite via thefile:<path>?mode=rwURI. Missing files surface a clear error thatreferences
--history-dband the env vars; no empty file is evercreated.
--history-dbflag and everycommand's
Longdescription;gpbackman/COMMANDS.mdandgpbackman/README.mdaligned.wrappers_test.go(env-var resolutionorder, explicit-wins-over-env) and in
utils_db_test.go(friendlyerror + no-file-created regression, happy-path open).
Backward compatibility
Fully compatible. The old "search the current directory" behaviour is
preserved as the last fallback, so any existing scripts that rely on
running gpbackman from a directory containing
gpbackup_history.dbcontinue to work unchanged. Users who currently pass
--history-dbexplicitly are unaffected.
The only user-visible behaviour change is that running gpbackman
against a non-existent path now errors immediately with a clear message
instead of producing a misleading "no such table: backups" error after
silently creating an empty file. This is the intended fix.
How tested
go test ./gpbackman/cmd/... ./gpbackman/gpbckpconfig/... ./gpbackman/textmsg/...— all pass.make build— five binaries built cleanly.cwd, no env → friendly error, no file created. ✅COORDINATOR_DATA_DIRECTORYpointing at a non-existent path → friendly error mentioning the resolved path. ✅COORDINATOR_DATA_DIRECTORYpointing at the coordinator data dir → backup list rendered without--history-db. ✅Notes
Signed-off-byincluded on both commits.GPBACKMAN_HISTORY_DBoverride).