talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159136089
##########
gpbackman/cmd/wrappers.go:
##########
@@ -81,12 +81,24 @@ func setLogLevelFile(level string) error {
return nil
}
+// getHistoryDBPath resolves the path to the gpbackup_history.db file.
+// When the --history-db flag is empty, fall back (in order) to the
+// COORDINATOR_DATA_DIRECTORY and MASTER_DATA_DIRECTORY environment variables
+// that the standard Cloudberry/Greenplum environment scripts export, so that
+// users running gpbackman from a sourced cluster shell do not need to repeat
+// the path on every invocation. As a last resort, return the bare filename
+// (resolved against the current working directory), preserving the previous
+// behaviour.
func getHistoryDBPath(historyDBPath string) string {
- var historyDBName = historyDBNameConst
if historyDBPath != "" {
return historyDBPath
}
- return historyDBName
+ for _, envVar := range historyDBEnvVars {
Review Comment:
Thanks for raising this — I dug into the deployment model to assess the risk:
- `make install` (`Makefile:108-123`) only places `gpbackman` on the
coordinator; `$COORDINATOR_DATA_DIRECTORY` (set by `greenplum_path.sh`) points
unambiguously at that one cluster's history DB. The "jump host with multiple
clusters' history files" scenario is non-standard.
- `backup-info` / `report-info` (read-only): wrong cluster's DB → operator
immediately sees unfamiliar timestamps and stops; no destructive action.
- `backup-delete` / `backup-clean` for **local** backups: SSH to segments
(`backup_delete.go:516`); the wrong cluster's hosts simply fail.
- `backup-delete --plugin-config` for **S3** backups: this is the real risk,
but only if the plugin config *and* history DB are both mismatched, *and*
deployment is non-standard.
- `history-clean`: writes only a local DB copy, no source-cluster impact.
**Compromise proposal**: limit the env-var fallback to **read-only
commands** (`backup-info`, `report-info`); destructive commands
(`backup-delete`, `backup-clean`, `history-clean`) keep the path explicit —
either `--history-db` or cwd. That preserves the standard-coordinator UX win
from #96 while removing the cross-cluster footgun for anything that mutates
state.
If you'd prefer a stricter `--auto-load-history-db` opt-in even for
read-only commands, happy to switch. WDYT?
--
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]
