fix(whpg-backup): verify table storage after locking and retry on a new snapshot

[haolinw]

Problem

whpg-backup exports its snapshot before it locks the tables. LOCK TABLE and the later reads resolve names against the live catalog, so a table whose storage was replaced in that window cannot be read consistently under the snapshot. With --leaf-partition-data the pg_aoseg relation enumerated under the snapshot no longer exists and getModCount aborts the whole backup:

[CRITICAL]:-ERROR: relation "pg_aoseg.pg_aoseg_xxxxx" does not exist (SQLSTATE 42P01)

Without that query the table is silently backed up empty.

Users may hit this with a nightly ALTER TABLE ... SET WITH (REORGANIZE=true) on an in-scope AO table. The rewrite swaps the AO auxiliary relations onto a temporary table and drops it, so the table keeps its OID but its pg_aoseg_<n> helper is renamed; the number in the error is the temporary table's OID. TRUNCATE, SET DISTRIBUTED BY, VACUUM FULL of a heap table, and DROP + CREATE under the same name have the same effect on the snapshot (VACUUM FULL of an append-optimized table compacts in place and keeps its storage). A check on table OIDs would not catch the rewrite case.

Fix

1. After LockTables, compare each locked table's snapshot relfilenode with pg_relation_filenode(), which reads the live catalog. Partitions are followed through pg_inherits, because a backup without --leaf-partition-data locks and copies the parent while the data lives in the children; LOCK TABLE on the parent locks them too, so the check is race-free for them as well. Locks are held, so the answer is final. Different means rewritten or truncated, NULL means dropped and recreated.
2. If anything changed: roll back, take a new synchronized snapshot (re-applying the session GUCs the rollback discards), and resolve the include and exclude lists again from the names the user gave, validation and partition expansion included, so a recreated table is found by its new OID and a partition created in the window is picked up. Then lock again. Up to three attempts by default, no new flag; the WHPGBACKUP_SNAPSHOT_ATTEMPTS environment variable tunes the count (1 disables the retry, an invalid value stops the backup before it starts).
3. Relations still changing after the last attempt, and the locked tables above them, leave the data backup set with a WARNING naming the relation and the reason; they get no IncrementalMetadata.AO entry, no statistics under --with-stats, and are listed in the backup report (data not backed up:). Whatever metadata the backup carries for them is unaffected: a regular table keeps its DDL, an extension config-dump table has none, a --data-only backup has none.
4. Metadata-only backups skip the check; the catalog alone stays consistent under the snapshot.

Behaviour is unchanged when no concurrent DDL happens: one extra catalog query after the locks.

Tests

• Unit (backup/queries_relation_test.go, backup/validate_test.go): result mapping of the storage check; the WHPGBACKUP_SNAPSHOT_ATTEMPTS guard.
• Integration (integration/changed_relations_test.go): detection for reorganize, heap VACUUM FULL, TRUNCATE, drop and recreate, several tables at once, and a partition changed below a locked parent; RetrieveAndProcessTables retry, include list resolved again after drop and recreate, include list expanded again after a partitioned table is recreated with more partitions, attempts exhausted, the parent leaving the data set when its partition keeps changing, session GUCs preserved across the retry.
• End to end (end_to_end/concurrent_ddl_test.go): real whpg-backup blocked behind an uncommitted rewrite, for AO and heap rewrite, TRUNCATE and reload, drop and recreate, an included partitioned table recreated with more partitions, a partition truncated while the backup copies the parent, three back-to-back rewrites with --with-stats, a single attempt via WHPGBACKUP_SNAPSHOT_ATTEMPTS=1, and rejected invalid values; every backup scenario is restored afterwards.

Verified on a WHPG 7 cluster. The full integration and end-to-end suites produce the same failure set as main in that environment. The 6.x path is exercised by the WHPG6 lane of the live suites workflow.

[Copilot]

🟡 Changes recommended

Two critical findings remain unresolved in partition-change detection and retry handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR improves whpg-backup consistency during concurrent DDL by validating relation storage, retrying snapshots, and reporting unstable data.

Changes:

• Adds storage validation and snapshot retries with refreshed filters.
• Omits unstable data and related metadata while preserving applicable DDL.
• Adds retry configuration and broad unit, integration, and end-to-end coverage.

File summaries

File	Summary
report/report.go	Reports tables whose data was omitted.
report/report_test.go	Tests skipped-table report output.
options/options.go	Restores original include filters for retries.
integration/integration_suite_test.go	Improves integration cleanup.
integration/changed_relations_test.go	Covers concurrent relation changes and retries.
end_to_end/concurrent_ddl_test.go	Covers end-to-end concurrent DDL scenarios.
backup/wrappers.go	Implements validation, retries, and filter refresh.
backup/validate.go	Validates snapshot-attempt configuration.
backup/validate_test.go	Tests retry configuration validation.
backup/queries_relations.go	Detects changed relation storage.
backup/queries_relation_test.go	Tests storage-change result mapping.
backup/queries_incremental.go	Omits incremental metadata for skipped tables.
backup/global_variables.go	Tracks retry and skipped-table state.
backup/display_report_test.go	Tests skipped-table report parsing.
backup/data.go	Filters skipped data and emits warnings.
backup/backup.go	Integrates retries, reporting, and statistics filtering.

Review findings:

• Critical (1 vote): Partition topology changes visible only after the snapshot can be missed, preventing include-list re-expansion.
• Critical (1 vote): The retry path can panic when asserting StringArray as pflag.SliceValue.

Review details

Suppressed comments (2)

backup/queries_relations.go:602

• utils.MINIMUM_GPDB5_VERSION is still 5.1.0 (utils/util.go:27), but pg_relation_filenode is not available on the supported GPDB 5.x server. This query will therefore fail before relation locking completes for every GPDB 5 backup, rather than preserving the existing compatibility; use the GPDB 5 storage lookup or branch/raise the supported minimum and add coverage for that path.

		pg_catalog.pg_relation_filenode(c.oid) AS currentrelfilenode

backup/queries_relations.go:626

• pg_inherits permits multiple parents (the existing suite creates grandchild INHERITS (child, other) in integration/options_integration_test.go:442), but this map retains only one parent per child. If a changed child is below two locked parents, whichever row is processed last overwrites the other, so Ancestors and changedRelations omit one parent and that parent's COPY can still read the inconsistent child data. Track all parent links and mark every ancestor (or otherwise skip every affected root).

			parents[row.Oid] = uint32(row.ParentOid.Int64)

• Files reviewed: 16/16 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical hierarchy-safety issues and the empty-statistics failure remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

backup/backup.go:201

• When every table in the backup is marked changed, tablesWithBackedUpData(metadataTables) is empty. The statistics queries build IN () through SliceToQuotedString, so --with-stats fails instead of completing without statistics as required; simply skipping backupStatistics would also leave plugin-backed backups without the statistics file expected later in DoBackup. Handle an empty table set while still creating the empty statistics artifact.

		backupStatistics(tablesWithBackedUpData(metadataTables))

backup/validate_test.go:15

• This new fmt import is in a separate group after the third-party imports, so the file is not formatted according to the repository's required goimports convention (README.md:87-92). Move it into the standard-library group with strings; otherwise the formatting/lint check will rewrite or reject this file.

	"fmt"
	. "github.com/onsi/ginkgo/v2"
	. "github.com/onsi/gomega"

• Files reviewed: 16/16 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Broad snapshot-retry and concurrent-DDL behavior spans backup, reporting, and integration paths and warrants final human review.

Review details

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved issues remain in partition detection, include-table retry handling, and test transaction cleanup.

Review details

Suppressed comments (3)

backup/queries_relations.go:596

• This recursive CTE only walks pg_class/pg_inherits rows visible in the backup snapshot. If a new partition is attached to an existing partitioned table after that snapshot, the new child is absent from inheritance, while the parent commonly has relfilenode = 0, so dropped stays empty and no retry is triggered; the parent COPY can then silently omit the new partition's rows. The check needs to compare the snapshot descendant set with the live descendant/attachment set and treat additions or topology changes as changed as well as relfilenode mismatches.

	WITH RECURSIVE inheritance(oid, parentoid) AS (
		SELECT c.oid, NULL::oid
		FROM pg_class c
		WHERE c.oid IN (%s)
		UNION ALL
		SELECT i.inhrelid, i.inhparent
		FROM pg_inherits i
			JOIN inheritance h ON i.inhparent = h.oid
	)

backup/wrappers.go:356

• --include-table is registered with flagSet.StringArray in options/flag.go:80, but a pflag StringArrayValue does not implement pflag.SliceValue (the interface used by string-slice flags). Consequently, any changed relation in an include-table backup reaches this assertion during refreshFilterLists and panics instead of retrying, so the advertised drop/recreate and partition-expansion retry paths fail. Reset the StringArray value using an API/type that supports StringArray while preserving the original user names before revalidation.

		err := flag.Value.(pflag.SliceValue).Replace(filterOptions.GetOriginalIncludedTables())

integration/changed_relations_test.go:125

• A retry runs restartBackupTransactions, which starts a transaction on every pool connection, but this cleanup only rolls back connection 0. After any retry test, connection 1 remains an open idle transaction with the old snapshot and catalog locks, leaking state into later specs and potentially interfering with their DDL/teardown. Roll back every non-nil pool transaction here.

		if connectionPool.Tx[0] != nil {
			_ = connectionPool.Rollback(0)
		}

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

backup/queries_relations.go must preserve all parent links so changes propagate to every locked ancestor.

Review details

Suppressed comments (1)

backup/queries_relations.go:626

• pg_inherits permits a child to have multiple parents (the existing include expansion supports this, e.g. integration/options_integration_test.go:406-434), but this map overwrites all but one parent when the CTE returns multiple edges. If that child changes, Ancestors and the later changedRelations propagation cover only one locked parent, so a COPY of another parent can still read the child under the stale snapshot. Preserve all parent links and propagate every locked ancestor, deduplicating the results.

		if row.ParentOid.Valid {
			parents[row.Oid] = uint32(row.ParentOid.Int64)

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Lite

[bluethumpasaurus]

Looks good.