-
Notifications
You must be signed in to change notification settings - Fork 521
Conversation
e807513 to
f9a3820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new FDB_TSDB_CTRL_SET_FORMAT_BAD_SEC_ONLY control command to the time-series database (TSDB) that enables selective formatting of only bad sectors instead of formatting the entire database. This feature aims to prevent data loss by preserving good sectors when corruption is detected during database initialization.
Key changes:
- Added
FDB_TSDB_CTRL_SET_FORMAT_BAD_SEC_ONLYconstant andformat_bad_sec_onlyfield to enable selective bad sector formatting - Implemented new
format_cbcallback function to format individual sectors - Added logic in
fdb_tsdb_initto iteratively check and format only bad sectors when the feature is enabled
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| inc/fdb_def.h | Added new control command constant FDB_TSDB_CTRL_SET_FORMAT_BAD_SEC_ONLY (0x0C) and format_bad_sec_only boolean field to fdb_db struct; reformatted alignment of control command definitions |
| src/fdb_tsdb.c | Implemented format_cb callback for selective sector formatting; added control case handler for the new command; added iterative bad sector detection and formatting logic in fdb_tsdb_init |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8782457 to
43febe1
Compare
onopche
commented
Dec 17, 2025
@armink thanks for the review! I've changed the logic a bit (essentially just copied the bad sector formatting logic over from fdb_tsdb.c for consistency), which should address the AI comments. Any chance you could re-review? Cheers!
armink
commented
Dec 18, 2025
My main concern is: in extreme cases, if only one sector is damaged and formatted, will this prevent the entire database from querying and inserting data?
onopche
commented
Dec 18, 2025
My main concern is: in extreme cases, if only one sector is damaged and formatted, will this prevent the entire database from querying and inserting data?
I don't believe so. My understanding that the sector will be erased and header will be written and marked as FDB_SECTOR_STORE_EMPTY so it shouldn't affect the rest of the sectors - that's the point of selective formatting.
AFAICS, it's exactly same logic in KVDB, right? That's where I copied it to TSDB:
Line 1599 in 8856961
armink
commented
Dec 31, 2025
KVDB does not require the order of Flash sector addresses, but TSDB does.
onopche
commented
Jan 11, 2026
KVDB does not require the order of Flash sector addresses, but TSDB does.
Hey, @armink sorry for a belated reply, was on a break last 2 weeks.
Sure, but TSDB has enough info (sector size) to carry on even if a sector is damaged/erased. I don't quite understand how it supposed to work with regular TSDB updates and random power offs - my understanding, with the current logic it's a very high chance of losing all data on every power off, isn't it (append request -> erase sector -> power off (before we had a chance to update the header))?
armink
commented
Jan 12, 2026
KVDB does not require the order of Flash sector addresses, but TSDB does.
Hey, @armink sorry for a belated reply, was on a break last 2 weeks.
Sure, but TSDB has enough info (sector size) to carry on even if a sector is damaged/erased. I don't quite understand how it supposed to work with regular TSDB updates and random power offs - my understanding, with the current logic it's a very high chance of losing all data on every power off, isn't it (append request -> erase sector -> power off (before we had a chance to update the header))?
Because each sector header in TSDB stores start and end timestamp information, if the timestamp information for a sector is missing, the time-based search function may fail.
onopche
commented
Jan 12, 2026
Because each sector header in TSDB stores start and end timestamp information, if the timestamp information for a sector is missing, the time-based search function may fail.
Sure, but it's still a better option than losing all data, right? For me, it's choice between "may fail" and "will always fail". If it's a concern, I could put this logic behind FDB_TSDB_CTRL_ setting (e.g. FDB_TSDB_CTRL_FORMAT_BAD_SEC_ONLY), the way I did it before:
#define FDB_TSDB_CTRL_SET_FORMAT_BAD_SEC_ONLY 0x0C /**< set format bad sectors only mode control command, this change MUST before database initialization */
and default it to off for those who prefer the existing logic.
359f8c4 to
d8f7e42
Compare
onopche
commented
Jan 23, 2026
Hey @armink, just some more thoughts on this. My understanding is that if a TSDB sector is erased during append operation, IT IS the latest sector in sequence. So if we just mark this sector EMPTY on the next reboot, it won't break the time sequence, isn't it?
+Added FDB_TSDB_CTRL_SET_FORMAT_BAD_SEC_ONLY setting to TSDB to format only bad sectors to prevent data loss
Just a follow up on #383 issue. I'd imagine logic like this roughly. What it does is that rather than formatting all sectors it targets only affected sectors and carries on.