You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "liang8283 (via GitHub)" <gi...@apache.org> on 2026/05/12 04:47:08 UTC
[PR] feat(backup): file-hash incremental detection for heap and AO tables [cloudberry-backup]
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: 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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6701614685
@woblerr Thanks, confirmed. Fixed in e90efe1d. The enum fix only covered one case of a wider problem: restore loads a skipped table's old data file into the newest column list, and some changes alter that list or how stored values are dumped without touching the data files or the table's DDL timestamp. Besides `ALTER TYPE ... ADD/DROP ATTRIBUTE`, the same happens when a column is added to a table whose row type another table uses as a column type, and when `DROP DOMAIN ... CASCADE` drops a column.
The enum-label hash is replaced by a hash of the table's own columns (number, name, type, typmod, dropped) and of the definitions of every enum and composite type they use, directly or through domains, arrays, nested composite, range and multirange types, read inside the backup snapshot. The table is skipped only if this hash matches too. The query runs under a savepoint; if it fails, no heap table is skipped.
While going over the whole feature again I also fixed a few related cases:
- A table is now skipped only if the previous restore plan has data for it. A table that belonged to an extension in the previous backup (whose data is not backed up) and was then removed from it with `ALTER EXTENSION ... DROP TABLE` was skipped and restored empty.
- Unlogged tables are never hashed: a regular `CHECKPOINT` does not write their buffers, so their files may not move when they change.
- Hashes are keyed by oid until they are matched with the table's name inside the snapshot.
- `gp_toolkit.gpbackup_file_info` is dropped after each hashing step instead of being left in `gp_toolkit`.
With your steps the incremental now includes `t`, and the restore returns `(10,20,)`. I also checked `DROP ATTRIBUTE ... CASCADE`, a composite nested in another composite, an array of a domain over the composite, the table row-type case, `DROP DOMAIN ... CASCADE`, the extension case, an unlogged table, `RENAME COLUMN` on a partitioned and on an inheritance parent, a typed table, and a full → incremental → incremental chain: each affected table is included and restored to match the source, and unrelated heap tables are still skipped. The earlier scenarios still behave as before.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4473030617
##########
backup/incremental.go:
##########
@@ -31,6 +33,22 @@ func FilterTablesForIncremental(lastBackupTOC, currentTOC *toc.TOC, tables []Tab
return filteredTables
}
+// heapTableUnchanged reports whether a non-AO table can be skipped. That needs a
+// file hash in both backups (recorded only with --heap-file-hash) and matching
+// hashes and DDL timestamps; anything else includes the table, as before.
+func heapTableUnchanged(lastBackupTOC, currentTOC *toc.TOC, fqn string) bool {
+ currentEntry, ok := currentTOC.IncrementalMetadata.Heap[fqn]
+ if !ok || currentEntry.FileHashMD5 == "" {
+ return false
+ }
+ previousEntry, ok := lastBackupTOC.IncrementalMetadata.Heap[fqn]
+ if !ok || previousEntry.FileHashMD5 == "" {
+ return false
+ }
+ return previousEntry.FileHashMD5 == currentEntry.FileHashMD5 &&
Review Comment:
A full backup of a heap table records the enum value pending. Running ALTER TYPE state RENAME VALUE 'pending' TO 'queued' leaves both the heap file hash and the table’s DDL timestamp unchanged, so the incremental skips the table. During restore, the enum contains queued, but COPY reads pending from the full backup and fails with invalid input value for enum public.state: "pending". An incremental from the same full backup without --heap-file-hash restores the row as queued.
--
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] feat(backup): file-hash incremental detection for heap and AO tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4437356598
##########
backup/wrappers.go:
##########
@@ -760,8 +760,64 @@ func backupTableStatistics(statisticsFile *utils.FileWithByteCount, tables []Tab
}
func backupIncrementalMetadata() {
+ // Always collect AO metadata via modcount + DDL timestamp.
aoTableEntries := GetAOIncrementalMetadata(connectionPool)
globalTOC.IncrementalMetadata.AO = aoTableEntries
+
+ useHeapHash := MustGetFlagBool(options.HEAP_FILE_HASH)
+ useAOHash := MustGetFlagBool(options.AO_FILE_HASH)
+
+ if !useHeapHash && !useAOHash {
+ gplog.Info("Collected incremental metadata: %d AO tables", len(aoTableEntries))
+ return
+ }
+
+ aoContentHashCount := 0
+ if useAOHash {
+ gplog.Info("Collecting AO aoseg content hashes (--ao-file-hash)")
+ aoContentHashes := GetAOContentHashes(connectionPool)
+ aoContentHashCount = len(aoContentHashes)
+ for fqn, hash := range aoContentHashes {
+ if existing, ok := aoTableEntries[fqn]; ok {
+ existing.FileHashMD5 = hash
+ aoTableEntries[fqn] = existing
+ }
+ }
+ globalTOC.IncrementalMetadata.AO = aoTableEntries
+ }
+
+ heapHashCount := 0
+ if useHeapHash {
+ if !ensureFileStatFunction(connectionPool) {
+ gplog.Warn("File hash setup incomplete, skipping heap file hash collection")
+ } else {
+ hashConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ hashConn.MustConnect(1)
+ defer hashConn.Close()
+
+ // CHECKPOINT flushes dirty pages so pg_stat_file sees up-to-date mtime/size.
Review Comment:
The heap hash can include writes that are not visible to the backup snapshot.
Holding a table lock, starting a full backup, and committing an insert after the backup established its snapshot but before its checkpoint and hash collection.
The full backup omitted the inserted row, while the next incremental saw the same hash and skipped the table.
Restore completed successfully but returned only id = 1, although the source contained both id = 1 and id = 2.
* Setup in a fresh test database
```sql
CREATE TABLE public.t(id int)
USING heap WITH (autovacuum_enabled=false)
DISTRIBUTED BY (id);
INSERT INTO public.t VALUES (1);
SELECT * FROM public.t;
CHECKPOINT;
```
* Session A — hold the table lock
```sql
BEGIN;
LOCK TABLE public.t IN ACCESS EXCLUSIVE MODE;
```
* Another terminal — start the full backup
```bash
gpbackup --dbname test \
--leaf-partition-data --heap-file-hash
...
```
* Session A — after backup is waiting at Acquiring `ACCESS SHARE` locks on tables
```sql
INSERT INTO public.t VALUES (2);
COMMIT;
```
* After the full backup finishes — incremental using its printed timestamp
```bash
gpbackup
--leaf-partition-data --heap-file-hash \
--incremental --from-timestamp '<FULL_TIMESTAMP>' \
...
```
* Restore using the incremental timestamp
```bash
gprestore --backup-dir /tmp/review98-backups \
--timestamp '<INCREMENTAL_TIMESTAMP>' \
...
```
##########
backup/incremental.go:
##########
@@ -13,17 +13,67 @@ import (
"github.com/pkg/errors"
)
+// FilterTablesForIncremental decides which tables need data backed up.
+//
+// Independent toggles:
+// - AO: default = modcount + DDL timestamp; with --ao-file-hash, use per-table aoseg content hash
+// - Heap: default = always include (original gpbackup behavior); with --heap-file-hash, skip unchanged via file hash
func FilterTablesForIncremental(lastBackupTOC, currentTOC *toc.TOC, tables []Table) []Table {
+ useAOHash := MustGetFlagBool(options.AO_FILE_HASH)
+ useHeapHash := MustGetFlagBool(options.HEAP_FILE_HASH)
+
var filteredTables []Table
for _, table := range tables {
- currentAOEntry, isAOTable := currentTOC.IncrementalMetadata.AO[table.FQN()]
- if !isAOTable {
+ fqn := table.FQN()
+
+ if currentAO, isAO := currentTOC.IncrementalMetadata.AO[fqn]; isAO {
+ if useAOHash && currentAO.FileHashMD5 != "" {
+ prevAO, hasPrev := lastBackupTOC.IncrementalMetadata.AO[fqn]
+ if !hasPrev || prevAO.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (AO/content) prev hash missing, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if prevAO.FileHashMD5 != currentAO.FileHashMD5 {
+ filteredTables = append(filteredTables, table)
+ }
+ } else {
+ prevAO := lastBackupTOC.IncrementalMetadata.AO[fqn]
+ if prevAO.Modcount != currentAO.Modcount || prevAO.LastDDLTimestamp != currentAO.LastDDLTimestamp {
+ filteredTables = append(filteredTables, table)
+ }
+ }
+ continue
+ }
+
+ if !useHeapHash {
+ // Original behavior: heap tables always included in incremental
filteredTables = append(filteredTables, table)
continue
}
- previousAOEntry := lastBackupTOC.IncrementalMetadata.AO[table.FQN()]
- if previousAOEntry.Modcount != currentAOEntry.Modcount || previousAOEntry.LastDDLTimestamp != currentAOEntry.LastDDLTimestamp {
+ // --heap-file-hash: skip heap tables whose file hash is unchanged
+ if currentTOC.IncrementalMetadata.Heap == nil {
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ currentHeap, isHeap := currentTOC.IncrementalMetadata.Heap[fqn]
+ if !isHeap || currentHeap.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (Heap) no current hash, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if lastBackupTOC.IncrementalMetadata.Heap == nil {
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ prevHeap, hasPrev := lastBackupTOC.IncrementalMetadata.Heap[fqn]
+ if !hasPrev || prevHeap.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (Heap) prev hash missing, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if prevHeap.FileHashMD5 != currentHeap.FileHashMD5 {
Review Comment:
The heap filter only compares file hashes, so it misses schema changes that do not rewrite the table. After a full backup with --heap-file-hash, dropping a non-key column left the heap hash unchanged in my test.
The incremental skipped the table and reused the previous data backup, whose COPY column list still included the dropped column.
Restore failed with column "payload" of relation "t" does not exist.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6704925197
There is one more issue with regclass columns. Could you take a look?
With a heap table storing `'public.target'::regclass`, taking a full backup and then running ALTER TABLE public.target RENAME TO renamed leaves the referencing table’s file hash, DDL timestamp and column hash unchanged, so the incremental skips it.
Restore then fails with relation "public.target" does not exist when loading the old reference from the full backup. An incremental from the same full backup without --heap-file-hash restores the reference successfully.
And branch conflicts need to be resolved.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4444634576
##########
backup/incremental.go:
##########
@@ -13,17 +13,67 @@ import (
"github.com/pkg/errors"
)
+// FilterTablesForIncremental decides which tables need data backed up.
+//
+// Independent toggles:
+// - AO: default = modcount + DDL timestamp; with --ao-file-hash, use per-table aoseg content hash
+// - Heap: default = always include (original gpbackup behavior); with --heap-file-hash, skip unchanged via file hash
func FilterTablesForIncremental(lastBackupTOC, currentTOC *toc.TOC, tables []Table) []Table {
+ useAOHash := MustGetFlagBool(options.AO_FILE_HASH)
+ useHeapHash := MustGetFlagBool(options.HEAP_FILE_HASH)
+
var filteredTables []Table
for _, table := range tables {
- currentAOEntry, isAOTable := currentTOC.IncrementalMetadata.AO[table.FQN()]
- if !isAOTable {
+ fqn := table.FQN()
+
+ if currentAO, isAO := currentTOC.IncrementalMetadata.AO[fqn]; isAO {
+ if useAOHash && currentAO.FileHashMD5 != "" {
+ prevAO, hasPrev := lastBackupTOC.IncrementalMetadata.AO[fqn]
+ if !hasPrev || prevAO.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (AO/content) prev hash missing, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if prevAO.FileHashMD5 != currentAO.FileHashMD5 {
+ filteredTables = append(filteredTables, table)
+ }
+ } else {
+ prevAO := lastBackupTOC.IncrementalMetadata.AO[fqn]
+ if prevAO.Modcount != currentAO.Modcount || prevAO.LastDDLTimestamp != currentAO.LastDDLTimestamp {
+ filteredTables = append(filteredTables, table)
+ }
+ }
+ continue
+ }
+
+ if !useHeapHash {
+ // Original behavior: heap tables always included in incremental
filteredTables = append(filteredTables, table)
continue
}
- previousAOEntry := lastBackupTOC.IncrementalMetadata.AO[table.FQN()]
- if previousAOEntry.Modcount != currentAOEntry.Modcount || previousAOEntry.LastDDLTimestamp != currentAOEntry.LastDDLTimestamp {
+ // --heap-file-hash: skip heap tables whose file hash is unchanged
+ if currentTOC.IncrementalMetadata.Heap == nil {
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ currentHeap, isHeap := currentTOC.IncrementalMetadata.Heap[fqn]
+ if !isHeap || currentHeap.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (Heap) no current hash, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if lastBackupTOC.IncrementalMetadata.Heap == nil {
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ prevHeap, hasPrev := lastBackupTOC.IncrementalMetadata.Heap[fqn]
+ if !hasPrev || prevHeap.FileHashMD5 == "" {
+ gplog.Debug("Filter: %s (Heap) prev hash missing, including", fqn)
+ filteredTables = append(filteredTables, table)
+ continue
+ }
+ if prevHeap.FileHashMD5 != currentHeap.FileHashMD5 {
Review Comment:
Thanks for the repro. `HeapEntry` now records the table's last DDL timestamp from `pg_stat_last_operation` (read inside the backup snapshot), and a heap table is skipped only if both the file hash and the DDL timestamp match the previous backup, the same rule AO tables already follow. With your DROP COLUMN scenario, the table is now included in the incremental and the restore succeeds with the new column list.
##########
backup/wrappers.go:
##########
@@ -760,8 +760,64 @@ func backupTableStatistics(statisticsFile *utils.FileWithByteCount, tables []Tab
}
func backupIncrementalMetadata() {
+ // Always collect AO metadata via modcount + DDL timestamp.
aoTableEntries := GetAOIncrementalMetadata(connectionPool)
globalTOC.IncrementalMetadata.AO = aoTableEntries
+
+ useHeapHash := MustGetFlagBool(options.HEAP_FILE_HASH)
+ useAOHash := MustGetFlagBool(options.AO_FILE_HASH)
+
+ if !useHeapHash && !useAOHash {
+ gplog.Info("Collected incremental metadata: %d AO tables", len(aoTableEntries))
+ return
+ }
+
+ aoContentHashCount := 0
+ if useAOHash {
+ gplog.Info("Collecting AO aoseg content hashes (--ao-file-hash)")
+ aoContentHashes := GetAOContentHashes(connectionPool)
+ aoContentHashCount = len(aoContentHashes)
+ for fqn, hash := range aoContentHashes {
+ if existing, ok := aoTableEntries[fqn]; ok {
+ existing.FileHashMD5 = hash
+ aoTableEntries[fqn] = existing
+ }
+ }
+ globalTOC.IncrementalMetadata.AO = aoTableEntries
+ }
+
+ heapHashCount := 0
+ if useHeapHash {
+ if !ensureFileStatFunction(connectionPool) {
+ gplog.Warn("File hash setup incomplete, skipping heap file hash collection")
+ } else {
+ hashConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ hashConn.MustConnect(1)
+ defer hashConn.Close()
+
+ // CHECKPOINT flushes dirty pages so pg_stat_file sees up-to-date mtime/size.
Review Comment:
Good catch. This was a real ordering bug: the hash was read after the backup snapshot was established. Heap file hashes are now collected in `DoSetup` on a separate connection (CHECKPOINT, then hash) **before** `initializeConnectionPool` begins the transactions and takes the snapshot. Any change committed after the hash is read moves the data files on, so the next incremental sees a different hash. At worst that is an extra copy, never a skipped change.
I ran your exact sequence (lock held by session A, full backup waiting on `ACCESS SHARE`, insert committed in the meantime): the incremental now includes the table, and the restore returns both `id = 1` and `id = 2`.
While looking at this I also tightened two related cases. If `CHECKPOINT` fails, no hashes are recorded. And because `pg_stat_file` reports mtime in whole seconds, a table whose file was modified in the same second as the hash read gets no hash. In both cases the table is included in the next incremental.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6599170919
@liang8283
There is another issue. Please, check it out.
The full backup hashes the heap file after CHECKPOINT flushes an uncommitted insert, but COPY cannot see that row. Committing the transaction later leaves the hash unchanged, so the next incremental skips the table.
In psql, create a heap table with one committed row, then leave the second insert uncommitted.
```sql
CREATE TABLE t(id int) USING heap DISTRIBUTED BY (id);
INSERT INTO t VALUES (1);
BEGIN;
INSERT INTO t VALUES (2);
```
In another terminal, run a full backup with --leaf-partition-data --heap-file-hash. Once it finishes, run COMMIT; in the original psql session.
Run an incremental from that full backup with --heap-file-hash and restore it. The restore succeeds, but `SELECT id FROM t ORDER BY id` returns only 1.
Repeating the incremental from the same full backup with --heap-file-hash disabled restores both 1 and 2.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6682547046
@liang8283 Thanks for fixing the previous issues.
Changing the structure of a composite type still leaves the heap file hash, the table’s DDL timestamp and the enum-label hash unchanged, so the incremental skips the table.
Restore then loads records from the full backup into the updated type and fails with malformed record literal. An incremental from the same full backup without `--heap-file-hash `restores successfully.
Create a test database and table.
```sql
CREATE TYPE pair AS (a int, b int);
CREATE TABLE t(id int, v pair)
USING heap WITH (autovacuum_enabled=false)
DISTRIBUTED BY (id);
INSERT INTO t VALUES (1, ROW(10,20)::pair);
SELECT * FROM t;
```
Take a full backup and note its printed timestamp.
Add an attribute to the composite type.
```sql
ALTER TYPE pair ADD ATTRIBUTE c int CASCADE;
SELECT * FROM t;
-- (10,20,)
```
Take an incremental using the full backup’s timestamp.
Restore using the incremental’s timestamp:
```
ERROR: malformed record literal: "(10,20)"
```
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6591394442
A follow-up on `--ao-file-hash`: I said earlier that it would come as a separate PR, but I'm dropping it instead. Looking at it again before the split, I found it is not safe, and on Cloudberry it brings no benefit over the existing `modcount` check:
- **DELETE is invisible to the aoseg hash.** Deleting rows from an AO/AOCS table only updates the visimap; `segno`, `eof`, `tupcount` and `vpinfo` in `pg_aoseg` / `pg_aocsseg` are unchanged, while `modcount` does increase. The hash replaced the `modcount` + DDL-timestamp comparison, so an AO table that only saw DELETEs would be skipped, and a restore would bring the deleted rows back. Dropping the DDL timestamp would also reintroduce the DROP COLUMN issue raised above, this time for AO tables.
- **No partition-granularity gain on Cloudberry.** The motivation was GP5, where a parent's `modcount` propagates to sibling partitions. On Cloudberry each leaf partition's `modcount` is already independent: inserting into one leaf leaves its siblings' `modcount` unchanged, so the current check already works per leaf.
- **The GP6 AOCS branch read columns that `pg_aocsseg` does not have** (`column_num`, `physical_segno`, `eof_uncompressed`), so that query would always fail on GP6.
Making it correct would mean adding `modcount` (or the visimap) back into the comparison, which leaves nothing to gain over today's behavior. So this PR stays `--heap-file-hash` only, and no AO follow-up is planned.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6619913547
@woblerr Thanks, confirmed and fixed in b60ef8f0. The heap files also contain rows of transactions that are still in progress, and their COMMIT only updates the commit log, so the hash never changed after the commit.
The fix: after reading the file hashes, and before the backup snapshot is taken, gpbackup now checks `pg_locks` and drops the hash of every table on which another transaction holds a lock taken by data-modifying commands (`RowExclusiveLock` and stronger). That lock is held until the writer's transaction ends, so the ordering covers both cases:
- the writer commits before the snapshot: its rows are visible to the backup;
- the writer is still open: it still holds the lock at the check, so the table gets no hash and the next incremental includes it.
If the lock check itself fails, no heap hashes are recorded at all.
With your exact steps (uncommitted insert during the full backup, committed after it), the incremental now includes `t` and the restore returns both 1 and 2. I also ran the same sequence with an uncommitted `UPDATE` and an uncommitted `DELETE` (which take `ExclusiveLock` with the global deadlock detector off): before this fix both were skipped by the incremental and restored the pre-commit values; now the table is included and the restore matches the source. The earlier scenarios (tail segments, snapshot race, DROP COLUMN, leaf partitions) still behave as before.
--
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] feat(backup): file-hash incremental detection for heap and AO tables [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4414704666
##########
backup/queries_incremental.go:
##########
@@ -176,3 +177,237 @@ func getLastDDLTimestamps(connectionPool *dbconn.DBConn) map[string]string {
}
return resultMap
}
+
+// heapTable carries the catalog identity of a heap table eligible for file-hash
+// incremental backup. Oid is what we pass into the file-hash plpgsql function
+// (pg_relation_filepath resolves it locally on each segment); FQN is for keying
+// the result map and for log messages.
+type heapTable struct {
+ Oid uint32
+ FQN string
+}
+
+// getFileHashesForTables collects file timestamp+size MD5 hashes for the given
+// heap tables using the provided connection. The connection must be reused
+// across all tables so gp_segment_id mapping stays consistent.
+func getFileHashesForTables(hashConn *dbconn.DBConn, tables []heapTable) map[string]string {
+ result := make(map[string]string)
+ for _, t := range tables {
+ hash := getTableFileHash(hashConn, t.Oid, t.FQN)
+ if hash != "" {
+ result[t.FQN] = hash
+ }
+ }
+ return result
+}
+
+// ensureFileStatFunction creates a plpgsql function (gp_toolkit.gpbackup_file_info)
+// that uses pg_relation_filepath() + pg_stat_file() to read each segment's
+// data-file mtime+size. Pure SQL, no plpython/shell. Uses a separate connection
+// so a failed setup does not abort the backup transaction.
+//
+// pg_relation_filepath is used because manual path construction is fragile:
+// custom tablespaces live under pg_tblspc/<tsp>/PG_<major>_<catver>/<db>/<rfn>,
+// and that PG_<major>_<catver> segment varies by server version. Letting the
+// server compute the path keeps us correct across versions and tablespace
+// configurations.
+func ensureFileStatFunction(connectionPool *dbconn.DBConn) bool {
+ gplog.Verbose("Setting up file hash detection function (plpgsql + pg_stat_file)")
+
+ setupConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ setupConn.MustConnect(1)
+ defer setupConn.Close()
+
+ checkSQL := `SELECT 1 AS val FROM pg_proc
+ WHERE proname = 'gpbackup_file_info'
+ AND pronargs = 1
+ AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'gp_toolkit');`
+ var checkResult []struct{ Val int }
+ err := setupConn.Select(&checkResult, checkSQL)
+
+ if err != nil || len(checkResult) == 0 {
+ gplog.Verbose("Creating gp_toolkit.gpbackup_file_info function")
+
+ // Drop any older (text, text) signature from a previous gpbackup version
+ // before recreating with the new (oid) signature.
+ _, _ = setupConn.Exec("DROP FUNCTION IF EXISTS gp_toolkit.gpbackup_file_info(text, text);", 0)
+
+ createSQL := `
+CREATE OR REPLACE FUNCTION gp_toolkit.gpbackup_file_info(p_oid oid)
+RETURNS text AS $BODY$
+DECLARE
+ v_path text;
+ v_mod text;
+ v_size text;
+BEGIN
+ v_path := pg_relation_filepath(p_oid);
+ IF v_path IS NULL THEN
+ RETURN '';
+ END IF;
+
+ BEGIN
+ SELECT (pg_stat_file(v_path)).modification::text,
+ (pg_stat_file(v_path)).size::text
+ INTO v_mod, v_size;
+ EXCEPTION WHEN OTHERS THEN
+ RETURN '';
+ END;
+
+ RETURN COALESCE(v_mod, '') || '|' || COALESCE(v_size, '0');
+END;
+$BODY$ LANGUAGE plpgsql;`
+
+ _, createErr := setupConn.Exec(createSQL, 0)
+ if createErr != nil {
+ gplog.Warn("Could not create gpbackup_file_info function: %v", createErr)
+ return false
+ }
+ gplog.Verbose("gpbackup_file_info function created successfully")
+ }
+
+ return true
+}
+
+// getHeapTables returns (oid, FQN) pairs of heap tables eligible for incremental backup.
+func getHeapTables(connectionPool *dbconn.DBConn) []heapTable {
+ var query string
+ if connectionPool.Version.IsGPDB() && connectionPool.Version.Before("7") {
+ query = fmt.Sprintf(`
+ SELECT c.oid,
+ quote_ident(n.nspname) || '.' || quote_ident(c.relname) AS tablefqn
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE c.relstorage = 'h'
+ AND c.relkind = 'r'
+ AND c.oid NOT IN (SELECT inhrelid FROM pg_inherits)
+ AND %s`, relationAndSchemaFilterClause())
+ } else {
+ query = fmt.Sprintf(`
+ SELECT c.oid,
+ quote_ident(n.nspname) || '.' || quote_ident(c.relname) AS tablefqn
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ JOIN pg_am a ON c.relam = a.oid
+ WHERE a.amname = 'heap'
+ AND c.relkind = 'r'
+ AND c.oid NOT IN (SELECT inhrelid FROM pg_inherits)
Review Comment:
**`NOT IN (SELECT inhrelid FROM pg_inherits)` excludes the leaf partitions of partitioned heap tables, so `--heap-file-hash` is effectively a no-op for them.** (Same clause in the pre-GP7 branch above.)
Verified against source and on a live cluster.
Partition children — whether created via `CREATE … PARTITION OF` or `ATTACH PARTITION` — are registered in `pg_inherits` as `inhrelid` by design (`commands/tablecmds.c`: `StoreCatalogInheritance(…, stmt->partbound != NULL)`, and `StoreCatalogInheritance1` on the attach path). So every heap leaf partition is an `inhrelid` and is filtered out here.
Live reproduction: a range-partitioned heap table with 3 leaves — the leaves are `relkind='r'`, `amname='heap'`, and all present in `pg_inherits.inhrelid`. Running this exact `getHeapTables` query returns only a non-partitioned control table; all 3 leaves are excluded (the root is already excluded by `relkind='r'`).
With leaves absent from `IncrementalMetadata.Heap`, `FilterTablesForIncremental` sees `isHeap=false` and falls back to "always include", so leaf partitions are always re-backed up. This is safe (over-backup, not data loss), but it means the feature does not apply to partitioned heap tables. Worth calling out — note the root partition was already excluded by `relkind='r'`, so this clause's net effect is to drop the leaves, which are the tables that actually carry data under `--leaf-partition-data`.
##########
backup/queries_incremental.go:
##########
@@ -176,3 +177,237 @@ func getLastDDLTimestamps(connectionPool *dbconn.DBConn) map[string]string {
}
return resultMap
}
+
+// heapTable carries the catalog identity of a heap table eligible for file-hash
+// incremental backup. Oid is what we pass into the file-hash plpgsql function
+// (pg_relation_filepath resolves it locally on each segment); FQN is for keying
+// the result map and for log messages.
+type heapTable struct {
+ Oid uint32
+ FQN string
+}
+
+// getFileHashesForTables collects file timestamp+size MD5 hashes for the given
+// heap tables using the provided connection. The connection must be reused
+// across all tables so gp_segment_id mapping stays consistent.
+func getFileHashesForTables(hashConn *dbconn.DBConn, tables []heapTable) map[string]string {
+ result := make(map[string]string)
+ for _, t := range tables {
+ hash := getTableFileHash(hashConn, t.Oid, t.FQN)
+ if hash != "" {
+ result[t.FQN] = hash
+ }
+ }
+ return result
+}
+
+// ensureFileStatFunction creates a plpgsql function (gp_toolkit.gpbackup_file_info)
+// that uses pg_relation_filepath() + pg_stat_file() to read each segment's
+// data-file mtime+size. Pure SQL, no plpython/shell. Uses a separate connection
+// so a failed setup does not abort the backup transaction.
+//
+// pg_relation_filepath is used because manual path construction is fragile:
+// custom tablespaces live under pg_tblspc/<tsp>/PG_<major>_<catver>/<db>/<rfn>,
+// and that PG_<major>_<catver> segment varies by server version. Letting the
+// server compute the path keeps us correct across versions and tablespace
+// configurations.
+func ensureFileStatFunction(connectionPool *dbconn.DBConn) bool {
+ gplog.Verbose("Setting up file hash detection function (plpgsql + pg_stat_file)")
+
+ setupConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ setupConn.MustConnect(1)
+ defer setupConn.Close()
+
+ checkSQL := `SELECT 1 AS val FROM pg_proc
+ WHERE proname = 'gpbackup_file_info'
+ AND pronargs = 1
+ AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'gp_toolkit');`
+ var checkResult []struct{ Val int }
+ err := setupConn.Select(&checkResult, checkSQL)
+
+ if err != nil || len(checkResult) == 0 {
+ gplog.Verbose("Creating gp_toolkit.gpbackup_file_info function")
+
+ // Drop any older (text, text) signature from a previous gpbackup version
+ // before recreating with the new (oid) signature.
+ _, _ = setupConn.Exec("DROP FUNCTION IF EXISTS gp_toolkit.gpbackup_file_info(text, text);", 0)
+
+ createSQL := `
+CREATE OR REPLACE FUNCTION gp_toolkit.gpbackup_file_info(p_oid oid)
+RETURNS text AS $BODY$
+DECLARE
+ v_path text;
+ v_mod text;
+ v_size text;
+BEGIN
+ v_path := pg_relation_filepath(p_oid);
Review Comment:
**`pg_relation_filepath(oid)` returns only the *first* 1GB segment, so this hash can silently miss changes on heap tables larger than 1GB.**
Checked against the kernel source and reproduced on a live cluster.
Source: a heap fork is split into `RELSEG_SIZE`-block segments (`storage/smgr/md.c`: `sprintf(segpath, "%s.%u", path, segno)`, with the comment *"Zero or more full segments of exactly RELSEG_SIZE blocks each"*), RELSEG_SIZE defaulting to 1GB. But `GetRelationPath` (`common/relpath.c`) never appends a `.N` suffix — the segment suffix is a storage-layer detail in `md.c` only. So `pg_relation_filepath` always yields the base segment, and `pg_stat_file` only stats that one file.
Live reproduction (single-segment heap table, all rows hashed to one segment):
- After loading 1.5GB: main segment = `1073741824 | 23:51:12` (exactly 1GB, full); `.1` segment = `511836160 | 23:51:13`.
- After appending 0.5GB more (lands in the tail segment, since the main one is already full) + `CHECKPOINT`: main segment = `1073741824 | 23:51:12` — **byte-for-byte identical mtime and size** — while `.1` grew to `1040351232 | 23:53:03`.
Since `gpbackup_file_info` hashes only the main segment, its hash is unchanged across those two states, so a >1GB append-mostly table is judged "unchanged" and skipped from the incremental, losing the new rows. The `size` fallback does not help, because a full segment's size is pinned at exactly 1GB.
The window is narrow (table >1GB, changes land only in tail segments, main segment untouched by UPDATE/VACUUM/hint-bits), but that is exactly the append-only large-table pattern incremental backup most wants to optimize.
Suggestion: stat every segment of the relation (`path`, `path.1`, `path.2`, … until `pg_stat_file` returns ENOENT, the way `md.c` walks segments) and fold each segment's mtime+size into the hash — or document the limitation / skip `--heap-file-hash` for tables over 1GB.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "woblerr (via GitHub) via commits" <co...@cloudberry.apache.org>.
woblerr commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6641450788
> @woblerr Thanks, confirmed and fixed in [b60ef8f](https://github.com/apache/cloudberry-backup/commit/b60ef8f06ceba9b70ba1e78f5f366157db4b84cc). The heap files also contain rows of transactions that are still in progress, and their COMMIT only updates the commit log, so the hash never changed after the commit.
>
> The fix: after reading the file hashes, and before the backup snapshot is taken, gpbackup now checks `pg_locks` and drops the hash of every table on which another transaction holds a lock taken by data-modifying commands (`RowExclusiveLock` and stronger). That lock is held until the writer's transaction ends, so the ordering covers both cases:
>
> * the writer commits before the snapshot: its rows are visible to the backup;
>
> * the writer is still open: it still holds the lock at the check, so the table gets no hash and the next incremental includes it.
>
>
> If the lock check itself fails, no heap hashes are recorded at all.
>
> With your exact steps (uncommitted insert during the full backup, committed after it), the incremental now includes `t` and the restore returns both 1 and 2. I also ran the same sequence with an uncommitted `UPDATE` and an uncommitted `DELETE` (which take `ExclusiveLock` with the global deadlock detector off): before this fix both were skipped by the incremental and restored the pre-commit values; now the table is included and the restore matches the source. The earlier scenarios (tail segments, snapshot race, DROP COLUMN, leaf partitions) still behave as before.
Thanks, the original case is fixed.
During the one-second wait after the incremental’s CHECKPOINT, I inserted id = 2 into a heap table and an AO table in the same transaction and committed.
By the time gpbackup checked pg_locks, the locks were gone and the heap file hash still matched the full backup. Restore succeeded, but only the AO table contained id = 2.
Could you check it out, please.
--
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] feat(backup): file-hash incremental detection for heap and AO tables [cloudberry-backup]
Posted by "talmacschen-arch (via GitHub)" <gi...@apache.org>.
talmacschen-arch commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6501759266
@liang8283 heads-up — I've reviewed this PR and left a Request Changes.
The one blocker is a correctness issue (F1): on heap tables larger than 1GB, `--heap-file-hash` stats only the first 1GB segment, because `pg_relation_filepath` never returns the `.N` tail segments (confirmed in `md.c` / `relpath.c`). I reproduced it on a live cluster — after appending 0.5GB to a >1GB table, the main segment's `size|mtime` was byte-for-byte unchanged while the new data went to `.1`, so an append-mostly large table would be silently skipped from the incremental. A fix suggestion (walk all segments like `md.c` does) is in the inline comment.
F2 (leaf partitions of partitioned heap tables are excluded via `NOT IN inhrelid`) and the missing unit tests for the new hash branches are non-blocking. The overall approach is solid — happy to re-review once the segment handling is sorted out. 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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4444631952
##########
backup/queries_incremental.go:
##########
@@ -176,3 +177,237 @@ func getLastDDLTimestamps(connectionPool *dbconn.DBConn) map[string]string {
}
return resultMap
}
+
+// heapTable carries the catalog identity of a heap table eligible for file-hash
+// incremental backup. Oid is what we pass into the file-hash plpgsql function
+// (pg_relation_filepath resolves it locally on each segment); FQN is for keying
+// the result map and for log messages.
+type heapTable struct {
+ Oid uint32
+ FQN string
+}
+
+// getFileHashesForTables collects file timestamp+size MD5 hashes for the given
+// heap tables using the provided connection. The connection must be reused
+// across all tables so gp_segment_id mapping stays consistent.
+func getFileHashesForTables(hashConn *dbconn.DBConn, tables []heapTable) map[string]string {
+ result := make(map[string]string)
+ for _, t := range tables {
+ hash := getTableFileHash(hashConn, t.Oid, t.FQN)
+ if hash != "" {
+ result[t.FQN] = hash
+ }
+ }
+ return result
+}
+
+// ensureFileStatFunction creates a plpgsql function (gp_toolkit.gpbackup_file_info)
+// that uses pg_relation_filepath() + pg_stat_file() to read each segment's
+// data-file mtime+size. Pure SQL, no plpython/shell. Uses a separate connection
+// so a failed setup does not abort the backup transaction.
+//
+// pg_relation_filepath is used because manual path construction is fragile:
+// custom tablespaces live under pg_tblspc/<tsp>/PG_<major>_<catver>/<db>/<rfn>,
+// and that PG_<major>_<catver> segment varies by server version. Letting the
+// server compute the path keeps us correct across versions and tablespace
+// configurations.
+func ensureFileStatFunction(connectionPool *dbconn.DBConn) bool {
+ gplog.Verbose("Setting up file hash detection function (plpgsql + pg_stat_file)")
+
+ setupConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ setupConn.MustConnect(1)
+ defer setupConn.Close()
+
+ checkSQL := `SELECT 1 AS val FROM pg_proc
+ WHERE proname = 'gpbackup_file_info'
+ AND pronargs = 1
+ AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'gp_toolkit');`
+ var checkResult []struct{ Val int }
+ err := setupConn.Select(&checkResult, checkSQL)
+
+ if err != nil || len(checkResult) == 0 {
+ gplog.Verbose("Creating gp_toolkit.gpbackup_file_info function")
+
+ // Drop any older (text, text) signature from a previous gpbackup version
+ // before recreating with the new (oid) signature.
+ _, _ = setupConn.Exec("DROP FUNCTION IF EXISTS gp_toolkit.gpbackup_file_info(text, text);", 0)
+
+ createSQL := `
+CREATE OR REPLACE FUNCTION gp_toolkit.gpbackup_file_info(p_oid oid)
+RETURNS text AS $BODY$
+DECLARE
+ v_path text;
+ v_mod text;
+ v_size text;
+BEGIN
+ v_path := pg_relation_filepath(p_oid);
Review Comment:
Thanks for tracking this down to `md.c` / `relpath.c`. Fixed in fbd27860: `gpbackup_file_info` now walks `path`, `path.1`, `path.2`, ... until `pg_stat_file` raises `undefined_file`, the same way `md.c` does, and folds each segment's mtime and size into the result. The relation path is part of the hash too, so a relfilenode change (TRUNCATE / VACUUM FULL / CLUSTER) is also detected.
I reproduced your scenario end to end: after a full backup, I appended to a >1GB table so that only `.1` grew, while the first segment kept its size (exactly 1GB) and mtime. The table is now included in the incremental, and the restored row count matches the source.
##########
backup/queries_incremental.go:
##########
@@ -176,3 +177,237 @@ func getLastDDLTimestamps(connectionPool *dbconn.DBConn) map[string]string {
}
return resultMap
}
+
+// heapTable carries the catalog identity of a heap table eligible for file-hash
+// incremental backup. Oid is what we pass into the file-hash plpgsql function
+// (pg_relation_filepath resolves it locally on each segment); FQN is for keying
+// the result map and for log messages.
+type heapTable struct {
+ Oid uint32
+ FQN string
+}
+
+// getFileHashesForTables collects file timestamp+size MD5 hashes for the given
+// heap tables using the provided connection. The connection must be reused
+// across all tables so gp_segment_id mapping stays consistent.
+func getFileHashesForTables(hashConn *dbconn.DBConn, tables []heapTable) map[string]string {
+ result := make(map[string]string)
+ for _, t := range tables {
+ hash := getTableFileHash(hashConn, t.Oid, t.FQN)
+ if hash != "" {
+ result[t.FQN] = hash
+ }
+ }
+ return result
+}
+
+// ensureFileStatFunction creates a plpgsql function (gp_toolkit.gpbackup_file_info)
+// that uses pg_relation_filepath() + pg_stat_file() to read each segment's
+// data-file mtime+size. Pure SQL, no plpython/shell. Uses a separate connection
+// so a failed setup does not abort the backup transaction.
+//
+// pg_relation_filepath is used because manual path construction is fragile:
+// custom tablespaces live under pg_tblspc/<tsp>/PG_<major>_<catver>/<db>/<rfn>,
+// and that PG_<major>_<catver> segment varies by server version. Letting the
+// server compute the path keeps us correct across versions and tablespace
+// configurations.
+func ensureFileStatFunction(connectionPool *dbconn.DBConn) bool {
+ gplog.Verbose("Setting up file hash detection function (plpgsql + pg_stat_file)")
+
+ setupConn := dbconn.NewDBConnFromEnvironment(connectionPool.DBName)
+ setupConn.MustConnect(1)
+ defer setupConn.Close()
+
+ checkSQL := `SELECT 1 AS val FROM pg_proc
+ WHERE proname = 'gpbackup_file_info'
+ AND pronargs = 1
+ AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'gp_toolkit');`
+ var checkResult []struct{ Val int }
+ err := setupConn.Select(&checkResult, checkSQL)
+
+ if err != nil || len(checkResult) == 0 {
+ gplog.Verbose("Creating gp_toolkit.gpbackup_file_info function")
+
+ // Drop any older (text, text) signature from a previous gpbackup version
+ // before recreating with the new (oid) signature.
+ _, _ = setupConn.Exec("DROP FUNCTION IF EXISTS gp_toolkit.gpbackup_file_info(text, text);", 0)
+
+ createSQL := `
+CREATE OR REPLACE FUNCTION gp_toolkit.gpbackup_file_info(p_oid oid)
+RETURNS text AS $BODY$
+DECLARE
+ v_path text;
+ v_mod text;
+ v_size text;
+BEGIN
+ v_path := pg_relation_filepath(p_oid);
+ IF v_path IS NULL THEN
+ RETURN '';
+ END IF;
+
+ BEGIN
+ SELECT (pg_stat_file(v_path)).modification::text,
+ (pg_stat_file(v_path)).size::text
+ INTO v_mod, v_size;
+ EXCEPTION WHEN OTHERS THEN
+ RETURN '';
+ END;
+
+ RETURN COALESCE(v_mod, '') || '|' || COALESCE(v_size, '0');
+END;
+$BODY$ LANGUAGE plpgsql;`
+
+ _, createErr := setupConn.Exec(createSQL, 0)
+ if createErr != nil {
+ gplog.Warn("Could not create gpbackup_file_info function: %v", createErr)
+ return false
+ }
+ gplog.Verbose("gpbackup_file_info function created successfully")
+ }
+
+ return true
+}
+
+// getHeapTables returns (oid, FQN) pairs of heap tables eligible for incremental backup.
+func getHeapTables(connectionPool *dbconn.DBConn) []heapTable {
+ var query string
+ if connectionPool.Version.IsGPDB() && connectionPool.Version.Before("7") {
+ query = fmt.Sprintf(`
+ SELECT c.oid,
+ quote_ident(n.nspname) || '.' || quote_ident(c.relname) AS tablefqn
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE c.relstorage = 'h'
+ AND c.relkind = 'r'
+ AND c.oid NOT IN (SELECT inhrelid FROM pg_inherits)
+ AND %s`, relationAndSchemaFilterClause())
+ } else {
+ query = fmt.Sprintf(`
+ SELECT c.oid,
+ quote_ident(n.nspname) || '.' || quote_ident(c.relname) AS tablefqn
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ JOIN pg_am a ON c.relam = a.oid
+ WHERE a.amname = 'heap'
+ AND c.relkind = 'r'
+ AND c.oid NOT IN (SELECT inhrelid FROM pg_inherits)
Review Comment:
Agreed. The `NOT IN (SELECT inhrelid FROM pg_inherits)` clause is removed from both branches, so leaf partitions are hashed like any other heap table. Verified with a range-partitioned heap table: after modifying one leaf, only that leaf was included in the incremental, and its siblings were skipped.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6581903944
Thanks both for the careful reviews and the reproductions. I pushed fbd27860, which addresses all of them; details are in the inline replies, and the PR description is updated.
- @talmacschen-arch: F1 (tail segments) and F2 (leaf partitions) are fixed. On the other points:
- Unit tests now cover the heap branches of `FilterTablesForIncremental`: unchanged, hash changed, DDL changed, and hash missing in either backup.
- `gpbackup_file_info` is recreated on every run, so a stale body is never reused.
- The helper now only treats a missing segment file as "end of relation"; any other error surfaces as a warning, and the table is included rather than silently hashed.
- The `CHECKPOINT` cost is now stated in the flag help and the PR description.
- @woblerr: both issues (DDL without a rewrite, and changes committed between snapshot and hash) are fixed, and your reproductions pass. As you suggested, I split the feature: this PR now contains only `--heap-file-hash`, and `--ao-file-hash` will come as a separate PR.
Could you take another look 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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6647797199
@woblerr Thanks, both confirmed and fixed in 9ef673aa.
**Commit during the wait after `CHECKPOINT`.** A row written after the `CHECKPOINT` can stay in shared buffers, so the file hash read a moment later does not see it, and once the writer commits before the lock check there is no lock left to catch it either. The hash is now verified after the backup snapshot is taken: gpbackup issues a second `CHECKPOINT`, re-reads the file info of every hashed table, and drops the hash of any table whose files changed since the first read. Any change committed before the snapshot is flushed by that second `CHECKPOINT`, so it can no longer hide behind a matching hash. The pre-snapshot lock check stays, since it covers rows written before the first `CHECKPOINT` and committed after the snapshot.
With your sequence (heap and AO insert committed in one transaction during the one-second wait), the incremental now includes the heap table and the restore returns the new row in both tables. To reproduce it reliably the row has to land in an existing page; a new row that extends the file moves its mtime immediately and was already caught.
**`ALTER TYPE ... RENAME VALUE`.** Enum values are stored by oid, so renaming a label changes the dumped text without touching the data files or the table's DDL timestamp. Each heap entry now also records a hash of the labels of the enum types its columns use, directly or through domains, arrays, composite types and range types, read inside the backup snapshot. A heap table is skipped only if the file hash, the DDL timestamp and this enum-label hash all match. With your scenario the table is included and restored as `queued`; I also checked a column of a domain-over-enum array and a composite type with an enum attribute, and an unrelated heap table in the same database is still skipped.
The earlier scenarios (uncommitted writes, tail segments, the lock-wait race, DROP COLUMN, leaf partitions) still behave as before.
--
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] feat(backup): file-hash incremental detection for heap tables [cloudberry-backup]
Posted by "liang8283 (via GitHub) via commits" <co...@cloudberry.apache.org>.
liang8283 commented on code in PR #98:
URL: https://github.com/apache/cloudberry-backup/pull/98#discussion_r4475816803
##########
backup/incremental.go:
##########
@@ -31,6 +33,22 @@ func FilterTablesForIncremental(lastBackupTOC, currentTOC *toc.TOC, tables []Tab
return filteredTables
}
+// heapTableUnchanged reports whether a non-AO table can be skipped. That needs a
+// file hash in both backups (recorded only with --heap-file-hash) and matching
+// hashes and DDL timestamps; anything else includes the table, as before.
+func heapTableUnchanged(lastBackupTOC, currentTOC *toc.TOC, fqn string) bool {
+ currentEntry, ok := currentTOC.IncrementalMetadata.Heap[fqn]
+ if !ok || currentEntry.FileHashMD5 == "" {
+ return false
+ }
+ previousEntry, ok := lastBackupTOC.IncrementalMetadata.Heap[fqn]
+ if !ok || previousEntry.FileHashMD5 == "" {
+ return false
+ }
+ return previousEntry.FileHashMD5 == currentEntry.FileHashMD5 &&
Review Comment:
Confirmed and fixed in 9ef673aa: each heap entry now also records a hash of the labels of the enum types the table uses (directly or through domains, arrays, composite and range types), and the table is skipped only if that matches too. With your scenario the table is included and restored as `queued`. Details in https://github.com/apache/cloudberry-backup/pull/98#issuecomment-6647797199.
--
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