This is an automated email from the ASF dual-hosted git repository. tuhaihe pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry-backup.git
commit 1035fb84eb85222b4df9b0ecaeb597f21d7a2b53 Author: chenqiang <[email protected]> AuthorDate: Wed Apr 29 12:45:47 2026 +0800 gpbackman: fall back to coordinator data dir for history db, fail loud 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 <[email protected]> --- gpbackman/cmd/backup_clean.go | 2 +- gpbackman/cmd/backup_delete.go | 2 +- gpbackman/cmd/backup_info.go | 2 +- gpbackman/cmd/constants.go | 8 ++++++ gpbackman/cmd/history_clean.go | 2 +- gpbackman/cmd/report_info.go | 2 +- gpbackman/cmd/root.go | 4 ++- gpbackman/cmd/wrappers.go | 16 ++++++++++-- gpbackman/cmd/wrappers_test.go | 45 ++++++++++++++++++++++++++++++++- gpbackman/gpbckpconfig/utils_db.go | 27 ++++++++++++++++++-- gpbackman/gpbckpconfig/utils_db_test.go | 38 ++++++++++++++++++++++++++++ 11 files changed, 137 insertions(+), 11 deletions(-) diff --git a/gpbackman/cmd/backup_clean.go b/gpbackman/cmd/backup_clean.go index 4f5e2179..1ccaf8f8 100644 --- a/gpbackman/cmd/backup_clean.go +++ b/gpbackman/cmd/backup_clean.go @@ -77,7 +77,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database will be searched in the current directory.`, +If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) diff --git a/gpbackman/cmd/backup_delete.go b/gpbackman/cmd/backup_delete.go index 14fa2795..202d52b3 100644 --- a/gpbackman/cmd/backup_delete.go +++ b/gpbackman/cmd/backup_delete.go @@ -84,7 +84,7 @@ For non local backups the following logic are applied: The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database will be searched in the current directory.`, +If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) diff --git a/gpbackman/cmd/backup_info.go b/gpbackman/cmd/backup_info.go index 199311cc..0515e402 100644 --- a/gpbackman/cmd/backup_info.go +++ b/gpbackman/cmd/backup_info.go @@ -98,7 +98,7 @@ To display the "object filtering details" column for all backups without using - The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database will be searched in the current directory.`, +If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) diff --git a/gpbackman/cmd/constants.go b/gpbackman/cmd/constants.go index 8259b594..14610c5a 100644 --- a/gpbackman/cmd/constants.go +++ b/gpbackman/cmd/constants.go @@ -72,4 +72,12 @@ var ( beforeTimestamp string // Timestamp to delete all backups after. afterTimestamp string + + // historyDBEnvVars lists, in priority order, the environment variables + // inspected when --history-db is not supplied. They are exported by the + // standard Cloudberry/Greenplum cluster environment scripts. + historyDBEnvVars = []string{ + "COORDINATOR_DATA_DIRECTORY", + "MASTER_DATA_DIRECTORY", + } ) diff --git a/gpbackman/cmd/history_clean.go b/gpbackman/cmd/history_clean.go index d5de643a..836991b8 100644 --- a/gpbackman/cmd/history_clean.go +++ b/gpbackman/cmd/history_clean.go @@ -50,7 +50,7 @@ Only --older-than-days or --before-timestamp option must be specified, not both. The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database will be searched in the current directory.`, +If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) diff --git a/gpbackman/cmd/report_info.go b/gpbackman/cmd/report_info.go index 3ac7428f..168695ae 100644 --- a/gpbackman/cmd/report_info.go +++ b/gpbackman/cmd/report_info.go @@ -78,7 +78,7 @@ It is not necessary to use the --plugin-report-file-path flag for the following The gpbackup_history.db file location can be set using the --history-db option. Can be specified only once. The full path to the file is required. -If the --history-db option is not specified, the history database will be searched in the current directory.`, +If the --history-db option is not specified, the history database is looked for under $COORDINATOR_DATA_DIRECTORY (then $MASTER_DATA_DIRECTORY); if neither variable is set, the current directory is used as a last resort.`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { doRootFlagValidation(cmd.Flags(), checkFileExistsConst) diff --git a/gpbackman/cmd/root.go b/gpbackman/cmd/root.go index 1c113b37..e8473269 100644 --- a/gpbackman/cmd/root.go +++ b/gpbackman/cmd/root.go @@ -50,7 +50,9 @@ func init() { &rootHistoryDB, historyDBFlagName, "", - "full path to the gpbackup_history.db file", + "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)", ) rootCmd.PersistentFlags().StringVar( &rootLogFile, diff --git a/gpbackman/cmd/wrappers.go b/gpbackman/cmd/wrappers.go index 79e34f38..be1c5b59 100644 --- a/gpbackman/cmd/wrappers.go +++ b/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 { + if dir := os.Getenv(envVar); dir != "" { + return filepath.Join(dir, historyDBNameConst) + } + } + return historyDBNameConst } func checkCompatibleFlags(flags *pflag.FlagSet, flagNames ...string) error { diff --git a/gpbackman/cmd/wrappers_test.go b/gpbackman/cmd/wrappers_test.go index e378fbf1..d7e16093 100644 --- a/gpbackman/cmd/wrappers_test.go +++ b/gpbackman/cmd/wrappers_test.go @@ -33,13 +33,56 @@ import ( var _ = Describe("wrappers tests", func() { Describe("getHistoryDBPath", func() { - It("returns default path when input is empty", func() { + // Save and restore env vars so these cases don't leak into the rest + // of the suite when run with --randomize-all. + var savedEnv map[string]string + + BeforeEach(func() { + savedEnv = make(map[string]string, len(historyDBEnvVars)) + for _, name := range historyDBEnvVars { + savedEnv[name] = os.Getenv(name) + os.Unsetenv(name) + } + }) + + AfterEach(func() { + for name, val := range savedEnv { + if val == "" { + os.Unsetenv(name) + } else { + os.Setenv(name, val) + } + } + }) + + It("returns default filename when input is empty and no env vars are set", func() { Expect(getHistoryDBPath("")).To(Equal(historyDBNameConst)) }) It("returns input path when not empty", func() { Expect(getHistoryDBPath("path/to/" + historyDBNameConst)).To(Equal("path/to/" + historyDBNameConst)) }) + + It("falls back to COORDINATOR_DATA_DIRECTORY when input is empty", func() { + os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") + Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) + }) + + It("falls back to MASTER_DATA_DIRECTORY when COORDINATOR is unset", func() { + os.Setenv("MASTER_DATA_DIRECTORY", "/master/data") + Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/master/data", historyDBNameConst))) + }) + + It("prefers COORDINATOR over MASTER when both are set", func() { + os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") + os.Setenv("MASTER_DATA_DIRECTORY", "/master/data") + Expect(getHistoryDBPath("")).To(Equal(filepath.Join("/coord/data", historyDBNameConst))) + }) + + It("explicit input wins over env vars", func() { + os.Setenv("COORDINATOR_DATA_DIRECTORY", "/coord/data") + Expect(getHistoryDBPath("/explicit/path.db")).To(Equal("/explicit/path.db")) + }) }) Describe("formatBackupDuration", func() { diff --git a/gpbackman/gpbckpconfig/utils_db.go b/gpbackman/gpbckpconfig/utils_db.go index 304c0907..9cf18df5 100644 --- a/gpbackman/gpbckpconfig/utils_db.go +++ b/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") if err != nil { return nil, err } diff --git a/gpbackman/gpbckpconfig/utils_db_test.go b/gpbackman/gpbckpconfig/utils_db_test.go index 17c6530b..a89e93e9 100644 --- a/gpbackman/gpbckpconfig/utils_db_test.go +++ b/gpbackman/gpbckpconfig/utils_db_test.go @@ -20,7 +20,10 @@ under the License. package gpbckpconfig import ( + "database/sql" "fmt" + "os" + "path/filepath" "github.com/apache/cloudberry-backup/history" . "github.com/onsi/ginkgo/v2" @@ -28,6 +31,41 @@ import ( ) var _ = Describe("utils_db tests", func() { + Describe("OpenHistoryDB", func() { + It("returns a friendly error and does not create a file when the path does not exist", func() { + tempDir := GinkgoT().TempDir() + missing := filepath.Join(tempDir, "does-not-exist.db") + + db, err := OpenHistoryDB(missing) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("not found")) + Expect(err.Error()).To(ContainSubstring("--history-db")) + Expect(db).To(BeNil()) + + // Critical regression: no empty SQLite file must have been created. + _, statErr := os.Stat(missing) + Expect(os.IsNotExist(statErr)).To(BeTrue(), "OpenHistoryDB must not create the file when it is missing") + }) + + It("opens an existing SQLite history database successfully", func() { + tempDir := GinkgoT().TempDir() + path := filepath.Join(tempDir, "gpbackup_history.db") + + // Seed an existing (but empty) SQLite file via the rwc URI mode. + seed, err := sql.Open("sqlite3", "file:"+path+"?mode=rwc") + Expect(err).NotTo(HaveOccurred()) + Expect(seed.Ping()).To(Succeed()) + Expect(seed.Close()).To(Succeed()) + + db, err := OpenHistoryDB(path) + Expect(err).NotTo(HaveOccurred()) + Expect(db).NotTo(BeNil()) + Expect(db.Ping()).To(Succeed()) + Expect(db.Close()).To(Succeed()) + }) + }) + Describe("getBackupNameQuery", func() { It("returns correct query for various flag combinations", func() { tests := []struct { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
