liang8283 opened a new pull request, #98: URL: https://github.com/apache/cloudberry-backup/pull/98
## Summary Adds two independent, opt-in flags to `gpbackup` that let `--incremental` detect table-level changes by hashing physical-storage state rather than relying solely on AO `modcount`: - **`--heap-file-hash`** — hashes each heap table's per-segment data-file mtime+size (via `pg_stat_file()` after a `CHECKPOINT`), so unchanged heap tables can be skipped from incremental for the first time. Default behavior — *always include heap* — is preserved when the flag is absent. - **`--ao-file-hash`** — hashes the AO/AOCS table's `pg_aoseg` content rows (`segno + eof + tupcount`, deliberately excluding `modcount`). This gives partition-leaf change detection in GP5 (where parent `modcount` propagates across sibling partitions) and catches some `modcount`-doesn't-match-data edge cases. AOCS uses a Cloudberry-aware column set (`vpinfo` when `!IsGPDB()`, otherwise the GP6+ schema). Both flags require `--leaf-partition-data` or `--incremental` (validated in `validate.go`). ## Motivation Two long-standing gaps in incremental backup: 1. **Heap tables had no change detection at all** — every incremental backup re-copied every heap table, regardless of whether the data file changed. For deployments with a few large mostly-static heap tables, this dominates incremental wall-time and storage. 2. **AO `modcount` is sometimes wrong** — on GP5-style storage, the parent partition's `modcount` is the sum across children, so a single child write makes every sibling appear changed. This PR ports the algorithm originally developed in `cloudberry-fe/gpbackup-enhanced` into Apache Cloudberry-backup with conventions cleaned up and lint compliance. ## Approach - **Catalog metadata for change detection lives in the TOC** — `IncrementalEntries.AO` already exists; this PR adds an optional `Heap` map and an optional `FileHashMD5` field to `AOEntry`, both `yaml:",omitempty"`. TOCs written without the new flags are bytewise identical to today's. Older `gprestore` reads new TOCs without issue (extra fields ignored). - **Hash collection runs on a dedicated `dbconn`** so a per-table query failure doesn't abort the backup transaction. The heap path also issues a `CHECKPOINT` first to flush dirty pages, so `pg_stat_file` mtime/size reflect data state and not just buffer state. - **The heap path uses `pg_relation_filepath(oid)`** rather than constructing the on-disk path manually — this avoids the very-easy-to-get-wrong `pg_tblspc/<oid>/PG_<major>_<catver>/...` path inside the plpgsql helper and is robust across PG/Cloudberry versions and tablespaces. - **OID is passed to the plpgsql helper** (not schema/table strings), so SQL escaping isn't a concern and identifiers with embedded quotes just work. - **`FilterTablesForIncremental` branches independently on each flag**. When neither flag is set, control flow is logically identical to the previous version (AO compared by `modcount + DDL timestamp`; heap unconditionally included). ## Files changed | File | Change | |---|---| | `toc/toc.go` | Add `HeapEntry`; add `IncrementalEntries.Heap` and `AOEntry.FileHashMD5` (both `omitempty`). | | `options/flag.go` | Add `HEAP_FILE_HASH`, `AO_FILE_HASH` constants and pflag registrations. | | `backup/queries_incremental.go` | New: `ensureFileStatFunction`, `getHeapTables`, `getTableFileHash`, `getFileHashesForTables`, `GetAOContentHashes`, `getAOSegContentHash`. plpgsql helper `gp_toolkit.gpbackup_file_info(p_oid oid)` uses `pg_relation_filepath(oid) + pg_stat_file()`. | | `backup/wrappers.go` | `backupIncrementalMetadata` now optionally `CHECKPOINT`s and collects heap + AO content hashes. | | `backup/incremental.go` | `FilterTablesForIncremental` rewrite with independent AO/heap branches. | | `backup/validate.go` | Reject `--heap-file-hash` / `--ao-file-hash` without `--leaf-partition-data` or `--incremental`. | | `backup/incremental_test.go` | Move `FilterTablesForIncremental` call into `JustBeforeEach` so package-level `cmdFlags` is initialized first. | ## Backward compatibility Fully compatible. - Without the new flags: behavior matches the prior release exactly. The default code path is reduced to the same comparison logic as the old `FilterTablesForIncremental`. - TOC schema additions are all `yaml:",omitempty"`. Old TOCs deserialize into the new structs; new TOCs without these flags serialize bytewise-identically to the old format. - `gp_toolkit.gpbackup_file_info` is only created when `--heap-file-hash` is used; if a previous gpbackup version installed an older `(text, text)` signature, the setup path drops it first (`DROP FUNCTION IF EXISTS ... (text, text)`). ## How tested - `make build`: 5 binaries built cleanly. - `make lint`: no new findings attributable to this change (pre-existing errcheck/unparam issues in unrelated files are unchanged). - `make unit`: all 13 Ginkgo suites pass. - `make end_to_end`: 162 / 196 active specs PASS / 0 regressions traceable to this change. The 34 failures are all environmental (`gpbackman` binary path, leftover `test_queue`/`test_tablespace` between specs, and one test that creates tables without `USING heap` on a cluster whose default access method is `ao_row`). - **Targeted live e2e on Cloudberry 2.5.0** — mixed schema with heap, AO row, AOCS, and partitioned AO; full + mutate + incremental cycle with both flags. Verified: changed tables included, unchanged tables skipped, partition-leaf granularity correct (one leaf included, sibling skipped), restore round-trip row counts match across all tables. - **Targeted live e2e on Synxdb 4.5.0-rc.3 with custom tablespace** — additionally verified that heap tables in a non-default tablespace are correctly detected as changed/unchanged. This is the empirical proof that `pg_relation_filepath()` handles the tablespace path correctly — without it, custom-tablespace heap would silently break in incremental detection. The e2e scripts used for verification are reproducible; happy to commit them into `end_to_end/` as a follow-up if reviewers want a regression test for the feature. ## Contributor's checklist - [x] Commits are logical units (one feat, one follow-up fix from code review). - [x] CI is expected to pass: `rat-check`, `build_and_unit_test`, smoke/unit/integration/end_to_end/s3_plugin_e2e/regression/scale. - [x] Code follows existing style; `gofmt` / `goimports` clean. - [ ] Squash if reviewers prefer a single commit — happy to do either. - [ ] `Signed-off-by` not currently included; will amend if reviewers want DCO. -- 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]
