fix(engine): seek_key must honor SET DELETED ON#4
Conversation
dbSeek of a key whose only matching record is deleted returned Found()==.T. (it landed on the deleted row). goto_top/goto_bottom/skip already consult show_deleted(); seek_key returned the raw B+tree match without checking it. After the B+tree positions, when deleted rows are hidden, skip forward over deleted records and re-derive `exact` from the row actually landed on (Clipper/DBFCDX semantics: if every matching row is deleted, Found() is .F. and the cursor sits on the next live record / Eof). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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 updates Table::seek_key in src/engine/table.cpp to correctly honor the SET DELETED ON setting by skipping over deleted records when performing a key seek. The review feedback highlights a potential bug where walking to the last matching key (such as in descending indexes) can still land on a deleted record, and suggests walking backward to find the last active matching record. Additionally, the feedback recommends simplifying the string padding and comparison logic using standard library functions like std::string::resize and operator== to improve readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ck.append(del_key.size() - ck.size(), ' '); | ||
| if (ck.size() > del_key.size()) ck.resize(del_key.size()); | ||
| exact = (std::memcmp(ck.data(), del_key.data(), del_key.size()) == 0); | ||
| } |
There was a problem hiding this comment.
When SET DELETED ON is active, if the seek lands on an exact match and walk_to_last is true (which is always the case for descending indexes or when last is true), the code walks forward to the last matching key without checking if those records are deleted. If the last matching record is deleted, the seek will land on it and report it as found, violating SET DELETED ON.
To fix this, after the walk_to_last loop finishes and loads the record, if SET DELETED ON is active and the loaded record is deleted, we should walk backward using idx->prev() until we find a non-deleted record. Since we started at a known active matching record, we are guaranteed to find one.
Here is how the walk_to_last block (lines 1083-1109) should be updated:
if (walk_to_last && exact) {
std::string padded_key = key;
padded_key.resize(idx->key_length(), ' ');
std::uint32_t last_recno = r.value().recno;
while (true) {
auto step = idx->next();
if (!step || !step.value().positioned) break;
std::string ck = idx->current_key();
ck.resize(padded_key.size(), ' ');
if (ck != padded_key) {
(void)idx->prev();
break;
}
last_recno = step.value().recno;
}
auto load = load_record_(last_recno);
if (!load) return load.error();
if (!openads::abi::show_deleted()) {
while (is_deleted()) {
auto step = idx->prev();
if (!step || !step.value().positioned) break;
if (auto ld = load_record_(step.value().recno); !ld) return ld.error();
}
}
last_seek_found_ = true;
return true;
}| if (!openads::abi::show_deleted()) { | ||
| std::string del_key = key; | ||
| if (del_key.size() < idx->key_length()) | ||
| del_key.append(idx->key_length() - del_key.size(), ' '); | ||
| if (del_key.size() > idx->key_length()) | ||
| del_key.resize(idx->key_length()); | ||
| while (r.value().positioned) { | ||
| if (auto ld = load_record_(r.value().recno); !ld) return ld.error(); | ||
| if (!is_deleted()) break; | ||
| r = idx->next(); | ||
| if (!r) return r.error(); | ||
| } | ||
| if (!r.value().positioned) { | ||
| state_ = (driver_->record_count() == 0) ? State::Limbo | ||
| : State::Eof; | ||
| recno_ = 0; | ||
| last_seek_found_ = false; | ||
| return false; | ||
| } | ||
| std::string ck = idx->current_key(); | ||
| if (ck.size() < del_key.size()) | ||
| ck.append(del_key.size() - ck.size(), ' '); | ||
| if (ck.size() > del_key.size()) ck.resize(del_key.size()); | ||
| exact = (std::memcmp(ck.data(), del_key.data(), del_key.size()) == 0); | ||
| } |
There was a problem hiding this comment.
The manual padding and resizing of del_key and ck can be simplified using std::string::resize(size, fill_char). This reduces boilerplate and makes the code much more readable. Additionally, we can use operator== for string comparison instead of std::memcmp.
if (!openads::abi::show_deleted()) {
std::string del_key = key;
del_key.resize(idx->key_length(), ' ');
while (r.value().positioned) {
if (auto ld = load_record_(r.value().recno); !ld) return ld.error();
if (!is_deleted()) break;
r = idx->next();
if (!r) return r.error();
}
if (!r.value().positioned) {
state_ = (driver_->record_count() == 0) ? State::Limbo
: State::Eof;
recno_ = 0;
last_seek_found_ = false;
return false;
}
std::string ck = idx->current_key();
ck.resize(del_key.size(), ' ');
exact = (ck == del_key);
}
|
Added a focused, self-contained smoke for this fix. It needs no fixtures — it What it exercises — table
Output (against a build that includes this patch): The driver is a small Harbour/RDD program. Happy to translate it into a |
Gemini review: after walking to the last equal-key entry, skip backward over deleted rows when SET DELETED ON is active. Simplify key padding with resize/operator==. Co-Authored-By: Grok Build (colab) <noreply@x.ai>
2 participants
Hi Antonio — as discussed, here is the seek/
SET DELETED ONfix.Problem
With
SET DELETED ON,dbSeekof a key whose only matching record isdeleted returned
Found() == .T.(it landed on the deleted row).dbGoTop/dbGoBottom/dbSkipand the index walk already skippeddeleted rows correctly — only seek did not.
Root cause
Table::goto_top/goto_bottom/skipconsultopenads::abi::show_deleted(), butTable::seek_keyreturned the rawB+tree match without checking it.
Fix
After the B+tree positions, when deleted rows are hidden, skip forward
over deleted records and re-derive
exactfrom the row actually landedon (Clipper/DBFCDX semantics: if every matching row is deleted,
Found()is
.F.and the cursor sits on the next live record / Eof).Single, additive change in
src/engine/table.cpp::seek_key; no behaviorchange when
SET DELETED OFF(the default).Testing
Developed and proven against a vendored OpenADS build driven by a Harbour
RDD CRUD harness over a legacy DBF/CDX dataset: with the fix, after
DELETE+SET DELETED ON,dbSeekon the deleted key returns.F.and the cursor lands on the next live record; index walk count is
unchanged. The patch applies cleanly to current
main(theseek_keylanding point and all primitives used —
load_record_,is_deleted(),idx->next()/current_key()/key_length(),show_deleted(),State::Limbo/Eof— are unchanged). Happy to add a focused smoke test ifyou'd like it in this PR.
Co-Authored-By: Claude Opus 4.8 (1M context)