You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "talmacschen-arch (via GitHub)" <gi...@apache.org> on 2026/04/29 05:01:36 UTC
[PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
talmacschen-arch opened a new pull request, #97:
URL: https://github.com/apache/cloudberry-backup/pull/97
Fixes #96
## Motivation
When `gpbackman backup-info` (or any other gpbackman subcommand) is run
without `--history-db`, the current code drops down to a relative
`gpbackup_history.db` resolved against the current directory. Two
problems follow:
1. The cluster's real history DB lives under
`$COORDINATOR_DATA_DIRECTORY/gpbackup_history.db` (or the legacy
`$MASTER_DATA_DIRECTORY` on older installs), which is rarely the
user's shell `cwd`. Operators end up typing the long path on every
invocation despite the relevant env vars already being set by the
standard cluster environment scripts.
2. 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 empty
`gpbackup_history.db` litter in `cwd`.
Reproducer (current behaviour on `main`):
```console
$ cd /tmp/empty
$ gpbackman backup-info
[ERROR]:-Unable to read data from history db. Error: no such table: backups
$ ls
gpbackup_history.db # ← silently created, 0 rows, polluted cwd
```
## Changes
* **`gpbackman/cmd/wrappers.go`** — `getHistoryDBPath` resolves the path
in order: `--history-db` → `$COORDINATOR_DATA_DIRECTORY` →
`$MASTER_DATA_DIRECTORY` → bare filename in `cwd` (preserved for
backward compatibility).
* **`gpbackman/gpbckpconfig/utils_db.go`** — `OpenHistoryDB` now
pre-checks the file with `os.Stat` and opens SQLite via the
`file:<path>?mode=rw` URI. Missing files surface a clear error that
references `--history-db` and the env vars; no empty file is ever
created.
* **Help text** updated for the `--history-db` flag and every
command's `Long` description; `gpbackman/COMMANDS.md` and
`gpbackman/README.md` aligned.
* **Tests** added/extended in `wrappers_test.go` (env-var resolution
order, explicit-wins-over-env) and in `utils_db_test.go` (friendly
error + 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.db`
continue to work unchanged. Users who currently pass `--history-db`
explicitly 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.
* Manual smoke test against a real cluster history DB:
* Empty `cwd`, no env → friendly error, no file created. ✅
* `COORDINATOR_DATA_DIRECTORY` pointing at a non-existent path → friendly error mentioning the resolved path. ✅
* `COORDINATOR_DATA_DIRECTORY` pointing at the coordinator data dir → backup list rendered without `--history-db`. ✅
## Notes
* DCO `Signed-off-by` included on both commits.
* See #96 for additional discussion / open questions (env-var name choice, optional `GPBACKMAN_HISTORY_DB` override).
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3161017739
##########
gpbackman/cmd/constants.go:
##########
@@ -72,4 +73,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",
Review Comment:
> I think we can remove MASTER_DATA_DIRECTORY support.
+1, remove MASTER_DATA_DIRECTORY support.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3161115763
##########
gpbackman/cmd/constants.go:
##########
@@ -72,4 +73,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",
Review Comment:
Done in 2d886893 — `MASTER_DATA_DIRECTORY` removed from the resolution
chain and from all related help/docs/tests. The `cmd/constants.go`
comment is also updated to drop "Greenplum". Thanks for the consensus!
Addressed in 2d886893 — removing `(or $MASTER_DATA_DIRECTORY)` made
the line naturally shorter.
Done in 2d886893 — `cmd/constants.go:79` now says "Cloudberry" only.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159300287
##########
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:
Implemented in 77e41407:
- New `--auto-load-history-db` persistent flag (default off). When off, behaviour matches upstream `main` — `getHistoryDBPath` returns `gpbackup_history.db` resolved against cwd. When on, the env-var chain (`$COORDINATOR_DATA_DIRECTORY` → `$MASTER_DATA_DIRECTORY` → cwd) kicks in.
- Destructive commands (`backup-delete`, `backup-clean`, `history-clean`) inherit the same flag, so they require either an explicit `--history-db` or a deliberate opt-in — the cross-cluster footgun is closed by default.
- Also folded in the wording fix on `wrappers.go:87` (dropped "Greenplum") and trimmed the README description per the other threads.
- The not-found error message now mentions `--auto-load-history-db` rather than implying that setting env vars alone is enough.
- Tests cover the opt-in matrix (off + env → cwd, on + env → env path, on + no env → cwd, explicit `--history-db` always wins).
The `OpenHistoryDB` safe-open change (`os.Stat` pre-check + `mode=rw` URI) from the original commits is preserved as-is. PTAL when you have a moment.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "woblerr (via GitHub)" <gi...@apache.org>.
woblerr commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3160539123
##########
gpbackman/cmd/constants.go:
##########
@@ -72,4 +73,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",
Review Comment:
Supporting a legacy variable doesn't make sense. The cloudberry project switched to using the `COORDINATOR_DATA_DIRECTORY` starting with version ~`1.5.4`.
I suggest using the current approach right away and not using legacy compatibility.
Similarly, it is worth removing the mention of `MASTER_DATA_DIRECTORY` in the rest of the code.
If there are reasons to maintain compatibility with `MASTER_DATA_DIRECTORY`, let's discuss it.
@MisterRaindrop @tuhaihe What do you think?
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#issuecomment-4343897961
@tuhaihe @woblerr — pushed 2d886893 with the MASTER_DATA_DIRECTORY
cleanup we agreed on (per @woblerr's request and @tuhaihe @MisterRaindrop's
+1). The new workflow runs are sitting in `action_required` (GitHub's
external-fork CI gate). Could one of you approve them when convenient?
cc @MisterRaindrop for visibility — feel free to re-review once CI clears.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3160695012
##########
gpbackman/cmd/constants.go:
##########
@@ -72,4 +73,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",
Review Comment:
Thanks for the context — that makes sense given Cloudberry 1.5.4+
standardized on COORDINATOR_DATA_DIRECTORY. The cleanup touches ~9 files
(code + long help strings + README/COMMANDS + tests), so I'd like to hold
off on pushing it as part of this PR until the maintainers have weighed
in on the overall direction. Happy to drop MASTER_DATA_DIRECTORY entirely
in a follow-up commit (or a separate PR) once that's confirmed.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159455595
##########
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, run gpbackman from "+
+ "the directory that contains gpbackup_history.db, or "+
+ "pass --auto-load-history-db to resolve it from "+
+ "$COORDINATOR_DATA_DIRECTORY (or $MASTER_DATA_DIRECTORY)",
+ historyDBPath,
+ )
+ }
+ return nil, err
Review Comment:
is better `return nil, fmt.Errorf("stat history db %q: %w", historyDBPath, err)`
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "woblerr (via GitHub)" <gi...@apache.org>.
woblerr commented on PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#issuecomment-4344953475
LGTM, thank you for your efforts.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159533991
##########
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, run gpbackman from "+
+ "the directory that contains gpbackup_history.db, or "+
+ "pass --auto-load-history-db to resolve it from "+
+ "$COORDINATOR_DATA_DIRECTORY (or $MASTER_DATA_DIRECTORY)",
+ historyDBPath,
+ )
+ }
+ return nil, err
Review Comment:
Good catch, applied in 70097db8 — wrapped with fmt.Errorf("stat history db %q: %w", historyDBPath, err) to keep errors.Is/As working while adding path/operation context. Thanks!
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#issuecomment-4348604301
LGTM
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159060634
##########
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:
OK
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3159300287
##########
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:
Implemented option B in 77e41407:
- New `--auto-load-history-db` persistent flag (default off). When off, behaviour matches upstream `main` — `getHistoryDBPath` returns `gpbackup_history.db` resolved against cwd. When on, the env-var chain (`$COORDINATOR_DATA_DIRECTORY` → `$MASTER_DATA_DIRECTORY` → cwd) kicks in.
- Destructive commands (`backup-delete`, `backup-clean`, `history-clean`) inherit the same flag, so they require either an explicit `--history-db` or a deliberate opt-in — the cross-cluster footgun is closed by default.
- Also folded in the wording fix on `wrappers.go:87` (dropped "Greenplum") and trimmed the README description per the other threads.
- The not-found error message now mentions `--auto-load-history-db` rather than implying that setting env vars alone is enough.
- Tests cover the opt-in matrix (off + env → cwd, on + env → env path, on + no env → cwd, explicit `--history-db` always wins).
The `OpenHistoryDB` safe-open change (`os.Stat` pre-check + `mode=rw` URI) from the original commits is preserved as-is. PTAL when you have a moment.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "tuhaihe (via GitHub)" <gi...@apache.org>.
tuhaihe commented on code in PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#discussion_r3160824005
##########
gpbackman/cmd/constants.go:
##########
@@ -72,4 +73,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",
Review Comment:
I think we can remove MASTER_DATA_DIRECTORY support.
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97#issuecomment-4343535656
@woblerr
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] gpbackman: env-var fallback for --history-db and fail loud when DB is missing [cloudberry-backup]
Posted by "tuhaihe (via GitHub)" <gi...@apache.org>.
tuhaihe merged PR #97:
URL: https://github.com/apache/cloudberry-backup/pull/97
--
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: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org