feat: support secure delete option.#34591
Conversation
xiao-77
requested review from
dapan1121,
guanshengliang and
hzcheng
as code owners
Summary of ChangesHello @xiao-77, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: secure data deletion. It enables the system to physically overwrite data in memory upon deletion, rather than just marking it for removal. This enhancement improves data privacy and compliance by making deleted information unrecoverable. The changes span multiple components, from parsing user commands to the core storage engine, ensuring that the secure delete option is correctly processed and applied throughout the data lifecycle. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a secureDelete option across the system, enabling physical data erasure instead of just marking it as deleted. This feature involves extensive changes to handle backward compatibility and implement core logic for secure in-memory data erasure. However, the current implementation has critical security concerns: the secure erase logic performs a linear scan of the entire memory table while holding a write lock, which could lead to a Denial of Service (DoS) on large tables. Additionally, using a predictable memory address to reseed the PRNG inside a loop is an insecure practice that could compromise the system's PRNG randomness. Furthermore, a logical flaw exists in how the secureDelete option is resolved for tables, where a table-level setting can be incorrectly overridden by the database-level setting.
source/dnode/mnode/impl/src/mndStb.c
Outdated
| pRsp->virtualStb = pStb->virtualStb; | ||
| pRsp->ownerId = pStb->ownerId; | ||
| pRsp->isAudit = pDb->cfg.isAudit ? 1 : 0; | ||
| pRsp->secureDelete = pStb->secureDelete ? pStb->secureDelete : pDb->cfg.secureDelete; |
There was a problem hiding this comment.
The current logic for determining the secureDelete setting for a table is flawed. If a table has secureDelete set to 0 (disabled), but its database has it set to 1 (enabled), this logic will result in pRsp->secureDelete being 1. This means the table-level setting is incorrectly overridden by the database-level setting. The table-level option should have precedence.
For example:
pStb->secureDelete= 0 (table has secure delete disabled)pDb->cfg.secureDelete= 1 (database has secure delete enabled)- The expression
pStb->secureDelete ? pStb->secureDelete : pDb->cfg.secureDeleteevaluates to0 ? 0 : 1, which is1. The table incorrectly gets secure delete enabled.
To fix this, secureDelete at the table level should probably be a tri-state value (e.g., -1 for inherit, 0 for off, 1 for on). If so, the logic here should be updated to correctly handle inheritance. This would also require ensuring pStb->secureDelete is initialized to a value indicating 'inherit' (e.g., -1) if not specified during table creation.
pRsp->secureDelete = (pStb->secureDelete != -1) ? pStb->secureDelete : pDb->cfg.secureDelete;| SMemSkipListNode *pNode = SL_NODE_FORWARD(pTbData->sl.pHead, 0); | ||
| while (pNode != pTbData->sl.pTail) { | ||
| TSDBROW *pRow = &pNode->row; | ||
| TSKEY rowTs = TSDBROW_TS(pRow); | ||
| if (rowTs >= sKey && rowTs <= eKey) { | ||
| if (pRow->type == TSDBROW_ROW_FMT) { | ||
| SRow *pSRow = pRow->pTSRow; | ||
| if (pSRow) { | ||
| int32_t len = pSRow->len > (int32_t)sizeof(SRow) ? pSRow->len - (int32_t)sizeof(SRow) : 0; | ||
| if (tsSecureEraseMode == 1) { | ||
| taosSeedRand((uint32_t)(uintptr_t)pSRow); | ||
| for (int32_t i = 0; i < len; i++) { | ||
| pSRow->data[i] = (uint8_t)(taosRand() & 0xFF); | ||
| } | ||
| } else { | ||
| (void)memset(pSRow->data, 0, len); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| pNode = SL_NODE_FORWARD(pNode, 0); | ||
| } |
There was a problem hiding this comment.
The tsdbSecureEraseMemTableData function performs a linear scan of the entire memory table skip list to find and overwrite rows within a specified timestamp range (sKey to eKey). This operation is performed while holding a write lock on the table data (pTbData->lock) in the calling function tsdbDeleteTableData (lines 207-209).
Because it iterates through every node in the skip list (SL_NODE_FORWARD(..., 0)) instead of using the skip list's search capabilities to find the start of the range, the complexity is O(N) where N is the total number of rows in the memory table. An attacker with DELETE permissions can exploit this by issuing DELETE ... SECURE_DELETE requests, especially with ranges that require scanning a large portion of the table, thereby blocking all other write operations on that table for an extended period. This constitutes a Denial of Service (DoS) vulnerability.
| if (pSRow) { | ||
| int32_t len = pSRow->len > (int32_t)sizeof(SRow) ? pSRow->len - (int32_t)sizeof(SRow) : 0; | ||
| if (tsSecureEraseMode == 1) { | ||
| taosSeedRand((uint32_t)(uintptr_t)pSRow); |
There was a problem hiding this comment.
When tsSecureEraseMode is set to 1, the code seeds the random number generator using the memory address of the row being erased: taosSeedRand((uint32_t)(uintptr_t)pSRow);.
Seeding a PRNG with a predictable value like a memory address inside a loop that iterates over many rows is a security weakness. If taosSeedRand and taosRand use a global state (which is common for PRNGs in C), this constant reseeding with predictable values can compromise the randomness of the PRNG for the entire process. This could lead to predictable values being generated in other parts of the system that rely on the same PRNG for security-sensitive operations (e.g., generating session tokens or nonces).
There was a problem hiding this comment.
Pull request overview
This pull request implements a secure delete feature that allows physical overwriting of data in memory when deleting records. The feature can be enabled at three levels: database configuration, table configuration, or per-DELETE statement using the SECURE_DELETE keyword. When enabled, deleted data in memory is overwritten with zeros or random data before being removed, providing additional data security.
Changes:
- Added SECURE_DELETE keyword and related SQL grammar for databases, tables, and DELETE statements
- Propagated secureDelete flag through all layers: parser → planner → executor → storage
- Implemented physical memory overwrite in tsdbMemTable when secure delete is enabled
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| source/libs/parser/inc/sql.y | Added SQL grammar for SECURE_DELETE keyword in database options, table options, and DELETE statements |
| source/libs/parser/src/parTokenizer.c | Added TK_SECURE_DELETE token definition |
| source/libs/parser/src/parAstCreater.c | Added AST node creation and validation for secureDelete option |
| source/libs/parser/src/parTranslater.c | Added validation for secureDelete database and table options |
| source/libs/parser/inc/parAst.h | Added enum values for secureDelete option types |
| source/libs/planner/src/planLogicCreater.c | Propagated secureDelete flag to logical plan nodes |
| source/libs/planner/src/planPhysiCreater.c | Propagated secureDelete flag to physical plan nodes |
| source/libs/executor/src/dataDeleter.c | Combined table-level and statement-level secureDelete flags using OR operator |
| source/libs/qworker/src/qworker.c | Added secureDelete field to delete result structure |
| source/dnode/vnode/src/tsdb/tsdbMemTable.c | Implemented secure erase of in-memory data, added tsdbSecureEraseMemTableData function |
| source/dnode/vnode/src/inc/vnodeInt.h | Updated tsdbDeleteTableData signature to include secureDelete parameter |
| source/dnode/vnode/src/vnd/vnodeSvr.c | Updated delete operation calls to pass secureDelete parameter |
| source/dnode/vnode/src/vnd/vnodeQuery.c | Added secureDelete to table configuration response |
| source/dnode/vnode/src/vnd/vnodeCfg.c | Added encoding/decoding of secureDelete in vnode configuration with backward compatibility |
| source/dnode/vnode/inc/vnode.h | Added secureDelete field to SVnodeCfg structure |
| source/dnode/mnode/impl/src/mndVgroup.c | Propagated secureDelete from database to vnode create/alter requests |
| source/dnode/mnode/impl/src/mndStb.c | Added secureDelete field to super table object, reduced STB_RESERVE_SIZE by 1 |
| source/dnode/mnode/impl/src/mndDb.c | Added secureDelete field to database configuration, reduced DB_RESERVE_SIZE by 1 |
| source/dnode/mnode/impl/inc/mndDef.h | Added secureDelete to SDbCfg and SStbObj structures |
| source/dnode/mgmt/mgmt_vnode/src/vmHandle.c | Propagated secureDelete in vnode management operations |
| source/common/src/tglobal.c | Added tsSecureEraseMode configuration parameter |
| source/common/src/msg/tmsg.c | Added serialization/deserialization for secureDelete in all relevant message types |
| include/util/tdef.h | Added TSDB_DEFAULT/MIN/MAX_DB_SECURE_DELETE constants |
| include/libs/nodes/querynodes.h | Added secureDelete field to SDeleteStmt |
| include/libs/nodes/plannodes.h | Added secureDelete fields to logic and physical plan nodes |
| include/libs/nodes/cmdnodes.h | Added secureDelete to SDatabaseOptions and STableOptions |
| include/libs/executor/dataSinkMgt.h | Added secureDelete fields to SDeleterRes and SDeleterParam |
| include/common/tmsg.h | Added secureDelete to multiple message structures |
| include/common/tglobal.h | Declared tsSecureEraseMode external variable |
| source/libs/command/src/command.c | Added SECURE_DELETE to SHOW CREATE DATABASE and SHOW CREATE TABLE output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| taosSeedRand((uint32_t)(uintptr_t)pSRow); | ||
| for (int32_t i = 0; i < len; i++) { | ||
| pSRow->data[i] = (uint8_t)(taosRand() & 0xFF); | ||
| } |
There was a problem hiding this comment.
Using pointer address as random seed is not cryptographically secure. The expression (uintptr_t)pSRow produces a predictable value based on memory layout. For secure deletion that aims to prevent data recovery, a cryptographically secure random number generator should be used instead. Consider using a proper CSPRNG or at least combining multiple entropy sources for the seed.
| @@ -85,6 +85,7 @@ static int32_t toDataCacheEntry(SDataDeleterHandle* pHandle, const SInputData* p | |||
| TAOS_STRCPY(pRes->tableName, pHandle->pDeleter->tableFName); | |||
| TAOS_STRCPY(pRes->tsColName, pHandle->pDeleter->tsColName); | |||
| pRes->affectedRows = *(int64_t*)pColRes->pData; | |||
There was a problem hiding this comment.
The bitwise OR operator combines the table-level and statement-level secureDelete flags. However, this means if either flag is set, secure delete will be enabled. This behavior should be documented in a comment, as it's not immediately obvious why OR is used instead of other logic (e.g., preferring the statement-level flag when explicitly set).
| pRes->affectedRows = *(int64_t*)pColRes->pData; | |
| pRes->affectedRows = *(int64_t*)pColRes->pData; | |
| /* Combine table-level and statement-level secureDelete flags: | |
| * secure delete is enabled if either flag is set. We use bitwise OR | |
| * intentionally here instead of preferring one flag over the other. */ |
|
|
||
| /************************************************ syncdb **************************************************************/ | ||
| cmd ::= DELETE FROM full_table_name(A) where_clause_opt(B). { pCxt->pRootNode = createDeleteStmt(pCxt, A, B); } | ||
| cmd ::= DELETE FROM full_table_name(A) where_clause_opt(B) SECURE_DELETE. { pCxt->pRootNode = createSecureDeleteStmt(pCxt, A, B); } |
There was a problem hiding this comment.
The SQL syntax places SECURE_DELETE at the end of the DELETE statement (after the WHERE clause). This is unconventional - typically, options are specified before or immediately after the table name. The current syntax "DELETE FROM table WHERE condition SECURE_DELETE" is unusual compared to standard SQL patterns. Consider whether this should be "DELETE SECURE_DELETE FROM table WHERE condition" or use a WITH clause like "DELETE FROM table WHERE condition WITH SECURE_DELETE" for better consistency with SQL conventions.
| cmd ::= DELETE FROM full_table_name(A) where_clause_opt(B) SECURE_DELETE. { pCxt->pRootNode = createSecureDeleteStmt(pCxt, A, B); } | |
| cmd ::= DELETE FROM full_table_name(A) where_clause_opt(B) WITH SECURE_DELETE. { pCxt->pRootNode = createSecureDeleteStmt(pCxt, A, B); } |
| // physically overwrite in-memory data when secure delete is requested | ||
| int8_t doSecureErase = secureDelete || pTsdb->pVnode->config.secureDelete; | ||
| if (doSecureErase) { | ||
| taosWLockLatch(&pTbData->lock); | ||
| tsdbSecureEraseMemTableData(pTbData, sKey, eKey); | ||
| taosWUnLockLatch(&pTbData->lock); | ||
| } |
There was a problem hiding this comment.
The secure delete feature only erases data in memory (memtable), but does not physically overwrite data already persisted to disk files. When data is flushed to disk during compaction, the deleted data marked in memory will be skipped, but the physical blocks on disk containing the deleted data are not overwritten. This means the feature provides incomplete secure deletion - data in memory is overwritten, but historical data already on disk remains recoverable through disk forensics. Consider documenting this limitation or extending the implementation to also securely erase disk-based data blocks when they are compacted.
| if (pRow->type == TSDBROW_ROW_FMT) { | ||
| SRow *pSRow = pRow->pTSRow; | ||
| if (pSRow) { | ||
| int32_t len = pSRow->len > (int32_t)sizeof(SRow) ? pSRow->len - (int32_t)sizeof(SRow) : 0; | ||
| if (tsSecureEraseMode == 1) { | ||
| taosSeedRand((uint32_t)(uintptr_t)pSRow); | ||
| for (int32_t i = 0; i < len; i++) { | ||
| pSRow->data[i] = (uint8_t)(taosRand() & 0xFF); | ||
| } | ||
| } else { | ||
| (void)memset(pSRow->data, 0, len); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The secure erase implementation only handles TSDBROW_ROW_FMT row type. If there are other row types that can exist in the skiplist, they will not be securely erased. Consider adding a comment explaining why other row types don't need secure erasure, or add handling for all row types to ensure complete secure deletion.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void tsdbSecureEraseMemTableData(STbData *pTbData, TSKEY sKey, TSKEY eKey) { | ||
| SMemSkipListNode *pNode = SL_NODE_FORWARD(pTbData->sl.pHead, 0); | ||
| while (pNode != pTbData->sl.pTail) { | ||
| TSDBROW *pRow = &pNode->row; | ||
| TSKEY rowTs = TSDBROW_TS(pRow); | ||
| if (rowTs >= sKey && rowTs <= eKey) { | ||
| if (pRow->type == TSDBROW_ROW_FMT) { | ||
| SRow *pSRow = pRow->pTSRow; | ||
| if (pSRow) { | ||
| int32_t len = pSRow->len > (int32_t)sizeof(SRow) ? pSRow->len - (int32_t)sizeof(SRow) : 0; | ||
| if (tsSecureEraseMode == 1) { | ||
| taosSeedRand((uint32_t)(uintptr_t)pSRow); | ||
| for (int32_t i = 0; i < len; i++) { | ||
| pSRow->data[i] = (uint8_t)(taosRand() & 0xFF); | ||
| } | ||
| } else { | ||
| (void)memset(pSRow->data, 0, len); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| pNode = SL_NODE_FORWARD(pNode, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tsdbSecureEraseMemTableData function only handles TSDBROW_ROW_FMT rows and silently skips TSDBROW_COL_FMT rows. However, column-format rows CAN exist in the in-memory skip list — they are inserted via tsdbInsertColDataToTable (tsdbMemTable.c line 696), which creates TSDBROW_COL_FMT rows in the skip list. As a result, in-memory column-format data is NOT overwritten when secure delete is requested, defeating the security purpose for data that was inserted in column format and hasn't been flushed to disk yet. The column-format row holds a pointer to an SBlockData in the vnode's buffer pool; to erase it, the column data arrays (e.g., pBlockData->aColData[i].pData) should be zeroed or randomized for target rows.
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU Affero General Public License, version 3 or later. |
There was a problem hiding this comment.
The license header block is duplicated in this file. Lines 4-6 contain the first license statement ("This program is free software: you can use, redistribute, and/or modify..."), and lines 8-9 repeat "This program is free software: you can redistribute it and/or modify..." — the same license sentence appears twice in slightly different forms. The second instance on lines 8-9 should be removed.
| * This program is free software: you can redistribute it and/or modify | |
| * it under the terms of the GNU Affero General Public License, version 3 or later. | |
| * | |
| * |
| if (doSecureErase) { | ||
| // Phase 1: overwrite in-memory (memtable) rows | ||
| taosWLockLatch(&pTbData->lock); | ||
| tsdbSecureEraseMemTableData(pTbData, sKey, eKey); | ||
| taosWUnLockLatch(&pTbData->lock); | ||
|
|
||
| // Phase 2: overwrite on-disk (data file + STT file) blocks. | ||
| // Errors are logged but not fatal: delete markers guarantee correctness | ||
| // even if the physical overwrite fails. | ||
| int32_t eraseCode = tsdbSecureEraseFileRange(pTsdb, suid, uid, sKey, eKey); | ||
| if (eraseCode != 0) { | ||
| tsdbWarn("vgId:%d, secure erase file range failed for suid:%" PRId64 " uid:%" PRId64 | ||
| " skey:%" PRId64 " eKey:%" PRId64 " since %s", | ||
| TD_VID(pTsdb->pVnode), suid, uid, sKey, eKey, tstrerror(eraseCode)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The secure erase of on-disk files (Phase 2, line 215) happens BEFORE the delete markers are added to the memtable (line 233). This means a concurrent reader that scans the file during the overwrite operation will find neither a delete marker (not yet written) nor the data (being overwritten). While this is a known limitation documented in P2 of the file header, the security window can be reduced by swapping the order: first add the delete marker to the memtable (so all subsequent reads see "deleted"), then perform the physical overwrite.
| " VIRTUAL %d", pCfg->virtualStb); | ||
| } | ||
|
|
||
| if (TSDB_SUPER_TABLE == pCfg->tableType) { |
There was a problem hiding this comment.
The appendTableOptions unconditionally appends SECURE_DELETE %d for every super table (line 888-891), even when secureDelete == 0 (disabled). This means SHOW CREATE TABLE for a super table will always show SECURE_DELETE 0, even when the option was never explicitly set. For consistency with how other options (e.g., TTL, KEEP, COMMENT) are handled — they are only shown when explicitly configured — consider only appending this option when pCfg->secureDelete != 0.
| if (TSDB_SUPER_TABLE == pCfg->tableType) { | |
| if (TSDB_SUPER_TABLE == pCfg->tableType && pCfg->secureDelete != 0) { |
| pOptions->keep = -1; | ||
| pOptions->virtualStb = false; | ||
| pOptions->commentNull = true; // mark null | ||
| pOptions->secureDelete = 0; |
There was a problem hiding this comment.
In createDefaultTableOptions, secureDelete is initialized to 0 (line 3159). This means when a super table is created without SECURE_DELETE, the default is 0 (disabled). This is consistent. However, note that in createAlterTableOptions (line 3173), secureDelete defaults to -1, indicating "no change". The buildCreateStbReq always writes pReq->secureDelete = pStmt->pOptions->secureDelete regardless of whether it was explicitly set. For CREATE TABLE, this means secureDelete = 0 is always sent, which is fine. But this is different from how other per-table options like keep (-1 = not set) are handled. Consider whether there is a need to distinguish between "explicitly set to 0" vs. "not specified" for CREATE TABLE.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * After adding delete markers to the memtable, scan all on-disk TSDB files | ||
| * (both DATA files and STT files) and physically overwrite the blocks that | ||
| * contain data for the target (suid, uid) in [sKey, eKey]. |
There was a problem hiding this comment.
The comment in the header of tsdbSecureErase.c (line 17) contradicts the actual implementation order: it says "After adding delete markers to the memtable, scan all on-disk TSDB files", but the implementation calls tsdbSecureEraseFileRange before the delete markers are added.
| * After adding delete markers to the memtable, scan all on-disk TSDB files | |
| * (both DATA files and STT files) and physically overwrite the blocks that | |
| * contain data for the target (suid, uid) in [sKey, eKey]. | |
| * In addition to applying logical delete markers (in the memtable and WAL), | |
| * scan all on-disk TSDB files (both DATA files and STT files) and physically | |
| * overwrite the blocks that contain data for the target (suid, uid) in [sKey, eKey]. |
| def test_delete_statement_with_secure_delete_option(self): | ||
| """Delete statement secure delete option | ||
|
|
||
| 1. Create database and super table with secure delete disabled | ||
| 2. Execute DELETE statement with explicit `SECURE_DELETE` | ||
| 3. Verify delete effect by row count | ||
|
|
||
| Since: v3.4.0.0 | ||
|
|
||
| Labels: common,ci | ||
|
|
||
| Jira: None | ||
|
|
||
| History: | ||
| - 2026-03-04 Codex Created | ||
| """ | ||
| tdLog.info("=== test delete statement with SECURE_DELETE option ===") | ||
| self._prepare_database("test_delete_stmt_secure_delete", 0) | ||
| self._prepare_stable("stb_delete_stmt_secure_delete", secure_delete=0) | ||
|
|
||
| tdSql.query("SELECT COUNT(*) FROM stb_delete_stmt_secure_delete") | ||
| tdSql.checkData(0, 0, 3) | ||
|
|
||
| tdSql.execute( | ||
| "DELETE FROM stb_delete_stmt_secure_delete WHERE ts <= now-5s SECURE_DELETE" | ||
| ) | ||
| tdSql.query("SELECT COUNT(*) FROM stb_delete_stmt_secure_delete") | ||
| tdSql.checkData(0, 0, 1) |
There was a problem hiding this comment.
The test for test_delete_statement_with_secure_delete_option only verifies the row count after delete (verifying the delete happened), but doesn't actually verify that the secure erase occurred (i.e., that data blocks were physically overwritten). The test is checking the logical deletion behavior, not the physical secure erasure which is the key feature. While physical file verification is harder in a Python test, at minimum the test should verify that the SQL syntax DELETE ... SECURE_DELETE is accepted and the delete actually removed data.
| /* time range check using key.ts (SBrinRecord.firstKey/lastKey are STsdbRowKey) */ | ||
| if (record.lastKey.key.ts < sKey || record.firstKey.key.ts > eKey) continue; | ||
| if (NULL == taosArrayPush(pRecords, &record)) { | ||
| code = terrno; |
There was a problem hiding this comment.
When taosArrayPush fails and execution jumps to _exit, tBrinBlockDestroy(&brinBlock) is not called for the current iteration's brinBlock. The brinBlock was populated by tsdbDataFileReadBrinBlock and contains allocated buffers that need to be freed. This causes a memory leak on the error path. A tBrinBlockDestroy(&brinBlock) call should be added before goto _exit.
| code = terrno; | |
| code = terrno; | |
| tBrinBlockDestroy(&brinBlock); |
| // secureDelete is merged by planner and vnode runtime config. | ||
| int8_t doSecureErase = secureDelete; | ||
| if (doSecureErase) { | ||
|
|
||
| // Phase 2: overwrite on-disk (data file + STT file) blocks. | ||
| // Errors are logged but not fatal: delete markers guarantee correctness | ||
| // even if the physical overwrite fails. | ||
| int32_t eraseCode = tsdbSecureEraseFileRange(pTsdb, suid, uid, sKey, eKey); | ||
| if (eraseCode != 0) { | ||
| tsdbWarn("vgId:%d, secure erase file range failed for suid:%" PRId64 " uid:%" PRId64 | ||
| " skey:%" PRId64 " eKey:%" PRId64 " since %s", | ||
| TD_VID(pTsdb->pVnode), suid, uid, sKey, eKey, tstrerror(eraseCode)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The file-level comment at the top of tsdbSecureErase.c says "After adding delete markers to the memtable, scan all on-disk TSDB files", but the actual code in tsdbDeleteTableData calls tsdbSecureEraseFileRange BEFORE writing the delete markers to the memtable. This ordering means that if the process crashes between the secure erase and the delete marker write, the physical data will be erased but no logical delete marker exists, leading to data corruption (zeroed/random-fill data without tombstone). The secure erase should happen after the delete markers are successfully committed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (NULL == taosArrayPush(pRecords, &record)) { | ||
| code = terrno; | ||
| goto _exit; | ||
| } | ||
| } | ||
| tBrinBlockDestroy(&brinBlock); |
There was a problem hiding this comment.
In tsdbSecureEraseDataFile, when taosArrayPush fails at the inner goto (around line 287), the code jumps to _exit without calling tBrinBlockDestroy(&brinBlock). The brinBlock variable (allocated by tsdbDataFileReadBrinBlock) is not in the _exit cleanup block, causing a memory/resource leak when the array push fails.
| // secureDelete is merged by planner and vnode runtime config. | ||
| int8_t doSecureErase = secureDelete; | ||
| if (doSecureErase) { | ||
|
|
||
| // Phase 2: overwrite on-disk (data file + STT file) blocks. | ||
| // Errors are logged but not fatal: delete markers guarantee correctness | ||
| // even if the physical overwrite fails. | ||
| int32_t eraseCode = tsdbSecureEraseFileRange(pTsdb, suid, uid, sKey, eKey); | ||
| if (eraseCode != 0) { | ||
| tsdbWarn("vgId:%d, secure erase file range failed for suid:%" PRId64 " uid:%" PRId64 | ||
| " skey:%" PRId64 " eKey:%" PRId64 " since %s", | ||
| TD_VID(pTsdb->pVnode), suid, uid, sKey, eKey, tstrerror(eraseCode)); | ||
| } | ||
| } | ||
|
|
||
| // do delete | ||
| SDelData *pDelData = (SDelData *)vnodeBufPoolMalloc(pPool, sizeof(*pDelData)); | ||
| if (pDelData == NULL) { |
There was a problem hiding this comment.
The comment labels the physical overwrite as "Phase 2: overwrite on-disk blocks" but there is no "Phase 1" comment in the function. This is confusing. Additionally, more critically, the physical erase happens BEFORE the delete markers are added to the memtable (which starts at line 194). If the memtable allocation fails (e.g., vnodeBufPoolMalloc returns NULL), the on-disk data will have been zeroed/randomized but no delete marker exists, causing data corruption: the overwritten data is now unreadable (due to CRC mismatch from the overwrite) but not logically deleted. The erase should either happen after the delete markers are committed or the function should abort and return an error if the erase fails in a way that leaves data in an inconsistent state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /* time range check using key.ts (SBrinRecord.firstKey/lastKey are STsdbRowKey) */ | ||
| if (record.lastKey.key.ts < sKey || record.firstKey.key.ts > eKey) continue; | ||
| if (NULL == taosArrayPush(pRecords, &record)) { | ||
| code = terrno; |
| SColCompressInfo cmprInfo = {.defaultCmprAlg = pTsdb->pVnode->config.tsdbCfg.compression}; | ||
| (void)metaGetColCmpr(pTsdb->pVnode->pMeta, bData.suid != 0 ? bData.suid : bData.uid, | ||
| &cmprInfo.pColCmpr); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| SColCompressInfo cmprInfo = {.defaultCmprAlg = pTsdb->pVnode->config.tsdbCfg.compression}; | ||
| (void)metaGetColCmpr(pTsdb->pVnode->pMeta, bData.suid != 0 ? bData.suid : bData.uid, | ||
| &cmprInfo.pColCmpr); |
| /* --------------------------------------------------------------------------- | ||
| * Public entry point. | ||
| * | ||
| * Called from tsdbDeleteTableData when secureDelete is enabled, after the |
| // Phase 2: overwrite on-disk (data file + STT file) blocks. | ||
| // Errors are logged but not fatal: delete markers guarantee correctness | ||
| // even if the physical overwrite fails. | ||
| int32_t eraseCode = tsdbSecureEraseFileRange(pTsdb, suid, uid, sKey, eKey); |
| int32_t newSize = 0; | ||
| for (int i = 0; i < 4; i++) newSize += (int32_t)buffers[i].size; | ||
|
|
||
| /* 5. Write new compressed block back at original offset */ | ||
| int64_t writeOff = blockOffset; | ||
| for (int i = 0; i < 4; i++) { | ||
| if (buffers[i].size) { | ||
| TAOS_CHECK_GOTO(tsdbWriteFile(pFD, writeOff, buffers[i].data, buffers[i].size, pEncryptData), | ||
| &lino, _exit); | ||
| writeOff += buffers[i].size; | ||
| } | ||
| } |
strcasestr is a POSIX extension not available on Windows. Replace it with a manual loop using strncasecmp (already used elsewhere in the file) to search for "VIRTUAL 1" as a word-boundary substring match. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /* time range check using key.ts (SBrinRecord.firstKey/lastKey are STsdbRowKey) */ | ||
| if (record.lastKey.key.ts < sKey || record.firstKey.key.ts > eKey) continue; | ||
| if (NULL == taosArrayPush(pRecords, &record)) { | ||
| code = terrno; |
| tBufferDestroy(&buffer); | ||
| tBufferDestroy(&assist); | ||
| for (int i = 0; i < 8; i++) tBufferDestroy(&buffers[i]); | ||
| if (cmprInfo.pColCmpr) taosHashCleanup(cmprInfo.pColCmpr); |
| /* --------------------------------------------------------------------------- | ||
| * Public entry point. | ||
| * | ||
| * Called from tsdbDeleteTableData when secureDelete is enabled, after the |
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.