gpbackman: env-var fallback for --history-db and fail loud when DB is missing#97
Merged
tuhaihe merged 5 commits intoapache:mainfrom Apr 30, 2026
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
ef444f4
Compare
…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>
…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.
woblerr
reviewed
Apr 29, 2026
Author
|
@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.
Author
|
@tuhaihe @woblerr — pushed 2d88689 with the MASTER_DATA_DIRECTORY cc @MisterRaindrop for visibility — feel free to re-review once CI clears. |
woblerr
approved these changes
Apr 29, 2026
Collaborator
|
LGTM, thank you for your efforts. |
|
LGTM |
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.
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).