Skip to content

fix: restore database delete skip for already deleted relations - #24748

Merged
XuPeng-SH merged 1 commit into
matrixorigin:mainfrom
gouhongshen:codex/restore-skip-deleted-relations-main
Jun 1, 2026
Merged

fix: restore database delete skip for already deleted relations#24748
XuPeng-SH merged 1 commit into
matrixorigin:mainfrom
gouhongshen:codex/restore-skip-deleted-relations-main

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

This reverts commit 0bfda60.

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #24711

What this PR does / why we need it:

Restore the main fix from #24717 after #24725 reverted it.

The original fix is still needed for restore-account database delete: when a relation was already deleted in the current transaction, Engine.Delete(database) should skip that relation only when tableOps.existAndDeleted proves it was deleted, while preserving other errors.

Tested with:

source $HOME/.zshrc && moenv && go test ./pkg/vm/engine/disttae

@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Restore database delete skip for already deleted relations

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Restore database delete skip for already deleted relations
• Add accountId field to txnDatabase struct
• Filter out relations already deleted in transaction
• Add comprehensive unit tests for relation deletion logic
Diagram
flowchart LR
  A["Engine.Delete database"] --> B["Get all relations"]
  B --> C["Filter already deleted relations"]
  C --> D["Delete remaining relations"]
  D --> E["Update transaction cache"]
  F["txnDatabase with accountId"] -.-> C
  G["tableOps.existAndDeleted check"] -.-> C

Loading

Grey Divider

File Changes

1. pkg/vm/engine/disttae/engine.go 🐞 Bug fix +30/-1

Add relation deletion filtering logic

• Add accountId field initialization in Create method
• Add accountId field initialization in Database method
• Implement filterDeleteDatabaseRelations function to skip already deleted relations
• Implement isDeleteDatabaseRelationDeletedInTxn helper function with system table account
 handling
• Apply filtering before deleting relations in Delete method

pkg/vm/engine/disttae/engine.go


2. pkg/vm/engine/disttae/engine_test.go 🧪 Tests +88/-0

Add unit tests for relation deletion filtering

• Add test for filterDeleteDatabaseRelations to verify deleted relations are skipped
• Add test for isDeleteDatabaseRelationDeletedInTxn with various account and table combinations
• Add test for system table account handling in catalog database
• Use mock transaction operator and table operations for testing

pkg/vm/engine/disttae/engine_test.go


3. pkg/vm/engine/disttae/types.go ✨ Enhancement +1/-0

Add accountId field to txnDatabase

• Add accountId field to txnDatabase struct

pkg/vm/engine/disttae/types.go


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 1, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Noisy skip-table logging 🐞 Bug ◔ Observability
Description
filterDeleteDatabaseRelations emits an Info log once per skipped relation, which can generate
high-volume logs and unnecessary overhead during database delete operations that skip many
already-deleted tables.
Code

pkg/vm/engine/disttae/engine.go[R761-767]

Evidence
Engine.Delete filters relations before deleting them and filterDeleteDatabaseRelations logs at
Info inside the per-relation loop whenever a relation is skipped, meaning one log entry per skipped
table.

pkg/vm/engine/disttae/engine.go[676-704]
pkg/vm/engine/disttae/engine.go[758-772]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`filterDeleteDatabaseRelations` logs an **Info** message for every table skipped because it was already deleted in the same transaction. During operations like restore-account cleanup, a database delete may skip many relations, producing excessive Info logs.

### Issue Context
The skip behavior is expected and non-error in normal execution; per-table Info logs can be noisy and add avoidable overhead.

### Fix Focus Areas
- pkg/vm/engine/disttae/engine.go[758-772]

### Suggested changes
- Downgrade this log to `Debug` (or gate it under `common.DoIfDebugEnabled`).
- Optionally aggregate: count skipped tables and emit a **single** Info log at the end (include database name, skipped count, and txn id).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 1, 2026
@mergify mergify Bot added the kind/bug Something isn't working label Jun 1, 2026
@XuPeng-SH
XuPeng-SH merged commit e29f6d1 into matrixorigin:main Jun 1, 2026
23 checks passed
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