talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159061735
##########
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:
OK
##########
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:
This seems to be a misunderstanding. Just to clarify the facts:
The original code sql.Open("sqlite3", path) (without URI) defaults to
mode=rwc (read + write + create) in go-sqlite3, which is why it was silently
creating empty files.
The new code file:path?mode=rw explicitly sets it to read + write, but
without create.
Write permissions are strictly necessary—operations like backup-delete,
backup-clean, and history-clean all require UPDATE/DELETE access to the history
DB.
The new code actually has narrower permissions than the original (removed
create), not broader. I suggest we reply politely to clarify this and keep the
code as is.
--
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]
