MisterRaindrop commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3158915240
##########
gpbackman/README.md:
##########
@@ -53,7 +53,7 @@ Available Commands:
Flags:
-h, --help help for gpbackman
- --history-db string full path to the gpbackup_history.db file
+ --history-db string full path to the gpbackup_history.db file
(if unset, falls back to $COORDINATOR_DATA_DIRECTORY/gpbackup_history.db, then
$MASTER_DATA_DIRECTORY/gpbackup_history.db, then the current directory)
Review Comment:
Suggest making it more concise
##########
gpbackman/gpbckpconfig/utils_db.go:
##########
@@ -21,15 +21,38 @@ package gpbckpconfig
import (
"database/sql"
+ "errors"
"fmt"
+ "os"
"strings"
"github.com/apache/cloudberry-backup/history"
)
-// OpenHistoryDB opens the history backup database.
+// OpenHistoryDB opens an existing gpbackup_history.db SQLite database.
+//
+// The path is opened with the SQLite "rw" URI mode so that a missing file
+// produces a clear error rather than being silently created as an empty
+// database (which would later fail with a confusing "no such table: backups"
+// when callers issue queries). Existence is also pre-checked with os.Stat to
+// surface a friendly error message that points the caller at the relevant
+// flag and environment variables.
func OpenHistoryDB(historyDBPath string) (*sql.DB, error) {
- db, err := sql.Open("sqlite3", historyDBPath)
+ if _, err := os.Stat(historyDBPath); err != nil {
+ if errors.Is(err, os.ErrNotExist) {
+ return nil, fmt.Errorf(
+ "gpbackup history database file not found: %s.
"+
+ "Specify the path via --history-db, set
"+
+ "COORDINATOR_DATA_DIRECTORY (or
MASTER_DATA_DIRECTORY), "+
+ "or run gpbackman from the directory
that contains "+
+ "gpbackup_history.db",
+ historyDBPath,
+ )
+ }
+ return nil, err
+ }
+ // mode=rw opens an existing database for read+write but never creates
one.
+ db, err := sql.Open("sqlite3", "file:"+historyDBPath+"?mode=rw")
Review Comment:
`db, err := sql.Open("sqlite3", historyDBPath)`
The original code didn’t grant read and write permissions.
Isn’t the permission granted here excessively broad?
##########
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:
Concern: when CWD, $COORDINATOR_DATA_DIRECTORY and $MASTER_DATA_DIRECTORY
all contain a gpbackup_history.db (common on a jump host managing
multiple clusters, or when a stale gpadmin profile is sourced), an
ordered-priority lookup becomes an implicit convention users have to
learn from docs. For destructive commands (backup-delete, history-clean)
that's a real footgun — the user thinks they're operating on cluster A
and silently hits cluster B's history DB.
We already have `--history-db` for the explicit case. If auto-discovery
is desired, I'd rather see it behind an opt-in flag (e.g.
`--auto-load-history-db` or `--from-coordinator-data-dir`) so the two
behaviors are clearly separated, and the default stays explicit.
##########
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
Review Comment:
`Cloudberry/Greenplum environment` remove Greenplum is better
--
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]
