Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6005 +/- ##
==========================================
+ Coverage 87.24% 87.44% +0.20%
==========================================
Files 197 217 +20
Lines 36012 39367 +3355
Branches 0 2634 +2634
==========================================
+ Hits 31420 34426 +3006
- Misses 4592 4932 +340
- Partials 0 9 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the cursor purge behavior by marking in-use cursors for deletion instead of deleting them immediately, ensuring safe concurrent cursor operations. Key changes include:
- Updating the cursor API to mark non-idle cursors for deletion.
- Adding new tests for both basic and ownership scenarios in the cursor API.
- Refactoring the cursor deletion and pause logic in the source code for clearer behavior and C++ compatibility.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/cpptests/test_cpp_cursors.cpp | Added tests to validate basic API behavior and ownership semantics. |
| src/cursor.h | Added C++ extern block and improved code comments regarding lock usage. |
| src/cursor.c | Refactored deletion/pause logic to support marking cursors for deletion safely. |
Comments suppressed due to low confidence (1)
src/cursor.c:286
- The new assignment to cur->pos omits the '- 1' adjustment present in the previous version, which may result in an off-by-one error when indexing into the idle array. Please verify that the ARRAY_ADD_AS macro returns the expected index so that cur->pos correctly references the newly added element.
cur->pos = ARRAY_GETSIZE_AS(&cl->idle, Cursor **);
alonre24
left a comment
alonre24
left a comment
There was a problem hiding this comment.
Good catch and a clean fix
Let's add a short stress test in this opportunity
| } | ||
|
|
||
| int Cursor_Free(Cursor *cur) { | ||
| return Cursors_Purge(getCursorList(cur->is_coord), cur->id); |
There was a problem hiding this comment.
Can we unify Cursors_Purge and Cursor_Free? Actually, neither of them is really freeing the memory, right? Better name would be Cursor_Delete IMO
There was a problem hiding this comment.
I didn't want to rename the function, so I don't have to change it in all the files that use it. It's not that bad in my opinion.
Cursors_Purge and Cursor_Free are NOT the same thing.
Cursors_Purgeis a request to delete a cursor, freeing it if it exists and is idle.Cursors_Freeis for releasing an owned cursor. It always freeing memory, and cannot fail.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the cursor purge logic to safely handle deletion of cursors that are actively used by other threads by marking them for deletion rather than deleting immediately.
- Updated the cursor deletion behavior in both pause and purge functions.
- Enhanced header documentation and added tests to verify the new behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/cpptests/test_cpp_cursors.cpp | Adds test cases to validate basic API calls and deletion-mark logic. |
| src/cursor.h | Updates the API documentation and adds the delete_mark flag to Cursor. |
| src/cursor.c | Modifies the free, pause, and purge functions to support deferred deletion. |
Comments suppressed due to low confidence (1)
src/cursor.c:260
- The previous implementation subtracted one from the idle array size to assign the cursor's position. Please verify that assigning cur->pos to the full size (without subtracting one) is intentional, so that the index stored correctly reflects the cursor's actual position in the idle list.
cur->pos = ARRAY_GETSIZE_AS(&cl->idle, Cursor **);
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-6005-to-2.8 origin/2.8
cd .worktree/backport-6005-to-2.8
git switch --create backport-6005-to-2.8
git cherry-pick -x 4f6000a4311babfd6e2cb720143fa0103cd1ef88 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.6
git worktree add -d .worktree/backport-6005-to-2.6 origin/2.6
cd .worktree/backport-6005-to-2.6
git switch --create backport-6005-to-2.6
git cherry-pick -x 4f6000a4311babfd6e2cb720143fa0103cd1ef88 |
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a)
|
Successfully created backport PR for |
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a)
|
Successfully created backport PR for |
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a)
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a)
Fix Cursor Purge - [MOD-9408] (#6005) * fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a) Co-authored-by: GuyAv46 <[email protected]>
* Fix Cursor Purge - [MOD-9408] (#6005) * fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff (cherry picked from commit 4f6000a) * fix test for 2.10 --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: GuyAv46 <[email protected]>
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff
* fix cursor purging mechanism * add new unit test for cursors * minor improvement * address CR * reduce diff
Describe the changes in the pull request
Fixing the cursors API so one can safely call
FT.CURSOR DELon some cursor currently being read by another connection on another thread.Instead of attempting to delete the requested cursor, if we see that it is currently being used, we mark it for deletion. When the using thread finishes with the cursor, it will delete the cursor even if it was not depleted.
The issue can only happen using
WORKERSor cluster mode (Coordinator).Which additional issues does this PR fix
Main objects this PR modified
Mark if applicable