Skip to content

fix: sync core-data delete - #5378

Open
remo-lab wants to merge 1 commit into
edgexfoundry:mainfrom
remo-lab:fix/core-data-delete-consistency
Open

fix: sync core-data delete#5378
remo-lab wants to merge 1 commit into
edgexfoundry:mainfrom
remo-lab:fix/core-data-delete-consistency

Conversation

@remo-lab

Copy link
Copy Markdown
Contributor

Summary

This PR fixes a core-data consistency bug where deleting telemetry by device name could make events and readings disappear from queries before the database delete had actually succeeded.

In event.go and deviceinfo.go, I removed the preemptive mark_deleted update on device_info, changed the PostgreSQL delete path to execute readings, events, and device_info cleanup in a
single transaction, and kept cache eviction only after commit. The async boundary stays in the application layer, so the API behaviors unchanged, but database failures now surface correctly instead of silently hiding still-present telemetry.

I also added a regression test in event_test.go for the transactional delete path.

Previously, DeleteEventsByDeviceName() marked matching device_info rows as deleted before the real delete work ran. Since event and reading queries filter on mark_deleted = false, any later failure
could leave data physically present in PostgreSQL but invisible to operators, while the delete request had already returned success.This change removes that silent-failure mode.

There are no API or schema changes.

Validation:

Passed:

  • GOCACHE=/tmp/go-build-cache go test ./internal/pkg/
    infrastructure/postgres
  • GOCACHE=/tmp/go-build-cache go test ./internal/core/data/...

Signed-off-by: remo-lab <remopanda7@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

@remo-lab

remo-lab commented Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

@judehung @weichou1229 @cloudxxx8 , please let me know your thoughts on this!

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 19.35484% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.33%. Comparing base (3148839) to head (cfc813c).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/pkg/infrastructure/postgres/event.go 19.35% 21 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5378      +/-   ##
==========================================
+ Coverage   21.21%   21.33%   +0.11%     
==========================================
  Files         285      285              
  Lines       20979    20966      -13     
==========================================
+ Hits         4451     4473      +22     
+ Misses      16174    16134      -40     
- Partials      354      359       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@remo-lab

Copy link
Copy Markdown
Contributor Author

@cloudxxx8 , shall i update anything on this pr?

@cloudxxx8

Copy link
Copy Markdown
Member

It's as design for the performance consideration, not a bug.
What is the real issue from your side?
If there are lots of events and reading received in Core Data every second and millions events / readings persisted in database, directly deletion may cause performance issue, especially the data is compressed.

@remo-lab

Copy link
Copy Markdown
Contributor Author

Hi @cloudxxx8, thanks for the feedback!

I understand the mark_deleted pattern is intentional for performance. The concern is: if updateDeviceInfosDeletableByDeviceName succeeds but the background delete transaction fails, the data becomes invisible (filtered by mark_deleted = false) yet physically remains in the DB — with no rollback or retry.

Would a smaller fix be acceptable instead — e.g., rolling back mark_deleted = false on the affected rows if the background delete fails? That preserves the performance optimization while closing the silent-failure window. Happy to update the PR accordingly!

@cloudxxx8

Copy link
Copy Markdown
Member

@remo-lab The logic in deleteEventsByConditions is async, so we cannot include the change of mark_deleted into single transaction. Also, write another db call to change mark_deleted is not a good idea, because it can be failed, either.

The reason we use async call for event deletion is that is really time consuming when the data volume is large.

However, your concern is valid. How about we add a periodical check to remove the data with mark_deleted=true?

// Purge the remaining device & none-device events not covered by retention policies and auto events

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants