Skip to content

fix(snapshot): reject RESTORE TABLE of referenced tables - #28126

Open
gouhongshen wants to merge 3 commits into
matrixorigin:mainfrom
gouhongshen:agent/issue-27117-9d395f95
Open

fix(snapshot): reject RESTORE TABLE of referenced tables#28126
gouhongshen wants to merge 3 commits into
matrixorigin:mainfrom
gouhongshen:agent/issue-27117-9d395f95

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Related to #27117

What this PR does / why we need it:

Root cause

RESTORE TABLE builds its foreign-key graph with filters for the selected table. For a referenced parent table, the child table that points to it is therefore not part of the table-restore graph. Later, recreateTable detects that the target is referenced and returns nil after logging a skip. The restore transaction then completes successfully without changing the target, leaving post-snapshot data visible.

Changes

  • Validate an explicit table-restore target after snapshot/account validation and before current foreign-key tables can be removed.
  • Return a shared not supported error when the target is referenced by a foreign-key constraint.
  • Propagate explicit table-restore mode through the recreation path and return metadata-check errors instead of continuing with a potentially stale result.
  • Preserve the existing skip behavior for database/account bulk restores, which may intentionally leave referenced master tables in place.
  • Add focused unit coverage for target validation, lookup-error propagation, explicit-versus-bulk recreation behavior, and the no-mutation early-return path.
  • Add a focused BVT for the issue reproduction.
  • Update the existing restore_fk_table.sql/.result case so explicit restores of referenced parent tables in both sys-account and acc01 contexts assert the same unsupported error while the subsequent data checks remain unchanged.

Issue-to-test proof

  • restore_table_referenced_by_fk.sql covers the ordinary table-restore control, historical snapshot read, referenced-parent restore, failure atomicity of the parent, child-row preservation, and cleanup. It asserts before for the control and historical read, requires the unsupported error for the parent restore, and observes changed plus child (1,1) afterward.
  • restore_fk_restore_master_table.sql verifies the existing account-level restore scenario now reports the same unsupported error and leaves the following table reads unchanged.
  • restore_fk_table.sql covers the two existing explicit parent-table restore cases in sys and non-system account sessions, asserting the unsupported error and retaining the original parent/child rows afterward; its later account restore continues to cover bulk-restore skipping.
  • TestRecreateTableReferencedByForeignKey covers explicit rejection versus bulk-restore skipping and metadata-check error propagation.
  • TestValidateRestoreTableTarget covers referenced targets, non-referenced targets, lookup errors, and target-account execution context.
  • TestRestoreTablesWithFkRejectsReferencedTable verifies explicit mode is propagated through the FK recreation helper.
  • TestRestoreTableRejectsReferencedTableBeforeMutation exercises doRestoreSnapshot end to end through validation, asserts the unsupported error, and verifies the transaction rolls back before any destructive SQL.

Tests run

  • make -C thirdparties -j2
  • make -C cgo -j2
  • make build -j2
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -run 'Test(RecreateTableReferencedByForeignKey|ValidateRestoreTableTarget|RestoreTablesWithFkRejectsReferencedTable|RestoreTableRejectsReferencedTableBeforeMutation)$' ./pkg/frontend
  • .agents/skills/mo-dev/scripts/mo-cgo-test ./pkg/frontend
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -coverprofile=/tmp/mo-27117-frontend.cover ./pkg/frontend
  • Properly configured mo-tester runs against an isolated standalone LOG/TN/CN cluster, all with a 100% threshold: restore_table_referenced_by_fk.sql 26/26, restore_fk_table.sql 289/289, and cluster/restore_fk_restore_master_table.sql 100/100.
  • Direct SQL execution of the issue reproduction against the same isolated cluster returned the explicit unsupported-operation error and left the changed parent row and child row unchanged.

Residual risks

  • RESTORE TABLE for a table referenced by a foreign key remains explicitly unsupported; implementing coordinated dependent-table restoration is outside this PR.
  • Database/account bulk restore keeps its existing master-table skip behavior.
  • PITR and unrelated restore paths are unchanged.

@gouhongshen-gmail

gouhongshen-gmail commented Sep 4, 2026

Copy link
Copy Markdown

Unrelated CI blockers: run 33844838948 had three self-hosted runners lose communication with GitHub; its AMD64 failures match #27784 and its ARM64 SCA failure matches #27742. The rerun was attempted once for attempt 1 and rejected with Must have admin rights to Repository; no second rerun was attempted.

A separate unrelated producer failure is tracked by #28171. In workflow run 33902380103, attempt 1, the compose multi-CN pessimistic BVT job 101122331742 failed only at window_hash_partition.sql row 18, analyze table t(k, ck);. The statement returned OK, but the report shows the expected legacy two-column NDV result versus the actual 11-column sampled-ANALYZE status row. The Coverage job 101135303754 was downstream (prerequisites_ready=false) and was not an independent root cause. PR #28126 does not touch ANALYZE, statistics, or window execution.

  • Run 33902380103 attempt 1: failed in BVT; the one permitted rerun advanced it to attempt 2
  • Attempt 2 BVT: job 101138384061 failed with the same window_hash_partition.sql row 18 oracle mismatch; its report again shows 14/15 success and the same 2-column versus 11-column result contract
  • Attempt 2 derived Coverage: job 101147760479, prerequisites_ready=false
  • Rerun status: attempt 2 repeated the same unrelated failure; no further rerun will be made

@gouhongshen
gouhongshen force-pushed the agent/issue-27117-9d395f95 branch from 95c450a to 7148dc5 Compare September 4, 2026 08:20
@gouhongshen
gouhongshen force-pushed the agent/issue-27117-9d395f95 branch from 7148dc5 to f345911 Compare September 4, 2026 08:32
@gouhongshen
gouhongshen force-pushed the agent/issue-27117-9d395f95 branch from 2f9ae43 to 9a9820d Compare September 4, 2026 10:59
@gouhongshen
gouhongshen force-pushed the agent/issue-27117-9d395f95 branch from 9a9820d to f0632df Compare September 4, 2026 11:52
@gouhongshen
gouhongshen force-pushed the agent/issue-27117-9d395f95 branch from f0632df to df7eb66 Compare September 4, 2026 12:14
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants