[liang8283]

Summary

Adds an opt-in --heap-file-hash flag to gpbackup so that --incremental can skip heap tables whose data has not changed. Today every heap table is re-copied by every incremental backup; with this flag, a heap table is skipped when both its on-disk file state and its last DDL timestamp match the previous backup.

Without the flag, behavior is unchanged: heap tables are always included, and AO tables keep using modcount + DDL timestamp.

--ao-file-hash from the first revision has been dropped: the aoseg content hash does not change on DELETE, so it is not a safe replacement for modcount (see the discussion below).

How it works

• What is hashed. A plpgsql helper, gp_toolkit.gpbackup_file_info(oid), runs on every segment via gp_dist_random. It resolves the relation's path with pg_relation_filepath() and stats every segment file of the main fork (path, path.1, path.2, ... until one is missing, the same way md.c walks them), so tables larger than RELSEG_SIZE are fully covered. The path itself is part of the hash, so a new relfilenode (TRUNCATE, VACUUM FULL, CLUSTER) is detected as a change.
• When it is hashed. Hashes are collected before the backup snapshot is taken, on a separate connection, right after a CHECKPOINT so that pg_stat_file sees flushed data. Any change committed after the hash is read moves the data files on, so the next incremental sees a different hash and includes the table. (Hashing after the snapshot would let changes the backup cannot see enter the hash.)
• DDL. Each heap entry also records the table's last DDL timestamp from pg_stat_last_operation, read inside the backup snapshot. A DDL that does not rewrite the table, such as DROP COLUMN, therefore still forces the table into the incremental.
• Partitions. Leaf partitions of partitioned heap tables are hashed like any other heap table.
• Failing safe. Whenever a hash cannot be trusted, no hash is recorded and the table is included in the next incremental:
  • CHECKPOINT fails;
  • a segment file was modified in the current second (pg_stat_file reports mtime in whole seconds, so a later write in the same second would not move it);
  • any segment cannot describe the relation, or the query fails.
• Decision. FilterTablesForIncremental skips a non-AO table only when both the previous and the current TOC carry a hash for it and both the hash and the DDL timestamp match.

Files changed

File	Change
toc/toc.go	Add HeapEntry{FileHashMD5, LastDDLTimestamp} and IncrementalEntries.Heap (omitempty).
options/flag.go	Add --heap-file-hash.
backup/backup.go	Collect heap file hashes in DoSetup, before the connection pool takes the snapshot.
backup/queries_incremental.go	CollectHeapFileHashes, ensureFileStatFunction, getHeapTables, getFileHashesForTables (batched), getHeapLastDDLTimestamps.
backup/wrappers.go	backupIncrementalMetadata writes heap entries (hash + DDL timestamp) into the TOC.
backup/incremental.go	FilterTablesForIncremental skips heap tables via heapTableUnchanged.
backup/validate.go	--heap-file-hash requires --leaf-partition-data.
backup/incremental_test.go	Unit tests for the heap branches: unchanged, hash changed, DDL changed, hash missing in either backup.

Backward compatibility

• Without the flag, the TOC and the incremental decision are identical to before.
• TOC additions are omitempty; older TOCs load fine, and older gprestore ignores the new fields.
• An incremental based on a backup taken without the flag includes every heap table (no previous hash).
• gp_toolkit.gpbackup_file_info is created only when the flag is used, and is replaced on each run so a stale body is never reused. The (text, text) signature from the first revision of this PR is dropped.

Operational notes

• The flag issues a cluster-wide CHECKPOINT at the start of each backup, plus a one-second wait so files flushed by it are not treated as "modified in the current second".
• Hashes are collected for all heap tables in the schemas being backed up; --include-table / --exclude-table are applied afterwards.

How tested

• make unit: all suites pass, including the new FilterTablesForIncremental heap tests.
• End to end on a Cloudberry cluster: full backup with --heap-file-hash, then mutations, then an incremental, then a restore of the incremental. Every table's row count matches the source, and:

Scenario	Result
Append to a heap table >1GB (first segment file unchanged, only .1 grows)	Included
Insert committed while the full backup waits for its table lock (after the snapshot)	Included; restore returns both rows
ALTER TABLE ... DROP COLUMN	Included; restore succeeds with the new column list
Partitioned heap table, one leaf modified	Modified leaf included, sibling leaves skipped
Unmodified heap table	Skipped

[talmacschen-arch]

Thanks for this — thoughtful PR and the overall design is sound: with both flags off the path is logically identical to the previous FilterTablesForIncremental, the TOC additions are all omitempty so backward compatibility holds, using a dedicated dbconn to avoid transaction-abort propagation is the right call, and excluding modcount from the AO content hash to dodge GP5's cross-partition modcount propagation addresses a real pain point.

I checked the two issues below against the Cloudberry kernel source and reproduced both on a live cluster, so they are not speculation — details are in the inline comments:

• [correctness] --heap-file-hash can silently miss changes on heap tables larger than 1GB — inline on gpbackup_file_info.
• [gap] --heap-file-hash does not cover leaf partitions of partitioned heap tables — inline on getHeapTables.

Two more points:

Test coverage: the new decision logic in FilterTablesForIncremental (AO-hash / heap-hash branches: prev vs current equal → skip, differ → include, either empty → include, Heap == nil → include) has no unit test — the test change only moves the existing flags-off call into JustBeforeEach. These branches are pure map logic over constructed TOCs and can be unit-tested without a live DB; worth adding since this is the heart of the feature.

nits:

• ensureFileStatFunction only CREATE OR REPLACEs when the function is absent; an older same-signature (oid) body would not be refreshed.
• The plpgsql EXCEPTION WHEN OTHERS THEN RETURN '' swallows every error (including permission failures) — safe-by-degradation, but hides real errors when debugging.
• CHECKPOINT is a cluster-wide heavy operation; frequent incrementals add I/O — worth a note in the docs.

Nice work overall — physical-state hashing for change detection is the right direction; both issues are about which physical state is being hashed.

[talmacschen-arch]

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.

[liang8283]

Thanks for tracking this down to md.c / relpath.c. Fixed in fbd2786: 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.

[talmacschen-arch]

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.

[liang8283]

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.

[talmacschen-arch]

Requesting changes on the strength of the correctness issue in my detailed review above (F1).

On heap tables larger than 1GB, --heap-file-hash stats only the first 1GB segment (pg_relation_filepath never returns the .N tail segments — confirmed in md.c/relpath.c), so append-only changes that land in tail segments are silently missed and dropped from the incremental. I reproduced this 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 data went to .1. That is a data-loss path for the feature.

Please either hash all segments of the relation (walk path, path.1, path.2, … like md.c does) or gate/document the flag for tables over 1GB before merge. F2 (leaf partitions excluded) and the missing unit tests are non-blocking. Happy to re-review once segment handling is addressed — the overall approach is good.

[talmacschen-arch]

@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!

[woblerr]

Preferably splitting --heap-file-hash and --ao-file-hash into two separate PRs. They are independent features, so each can be reviewed and merged on its own.

The --heap-file-hash functionality looks like the most useful change.

[woblerr]

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.

[liang8283]

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.

[woblerr]

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

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

BEGIN;
LOCK TABLE public.t IN ACCESS EXCLUSIVE MODE;

• Another terminal — start the full backup

gpbackup --dbname test \
  --leaf-partition-data --heap-file-hash 
  ...

• Session A — after backup is waiting at Acquiring ACCESS SHARE locks on tables

INSERT INTO public.t VALUES (2);
COMMIT;

• After the full backup finishes — incremental using its printed timestamp

gpbackup 
  --leaf-partition-data --heap-file-hash \
  --incremental --from-timestamp '<FULL_TIMESTAMP>' \
...

• Restore using the incremental timestamp

gprestore --backup-dir /tmp/review98-backups \
  --timestamp '<INCREMENTAL_TIMESTAMP>' \
...

[liang8283]

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.

[liang8283]

Thanks both for the careful reviews and the reproductions. I pushed fbd2786, 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?

[liang8283]

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.

[woblerr]

@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.

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.

[liang8283]

@woblerr Thanks, confirmed and fixed in b60ef8f. 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.