Fix concurrency bugs and UDF correctness issues found in PR #76 review#78
Closed
Fix concurrency bugs and UDF correctness issues found in PR #76 review#78
Conversation
- Move BITS_PER_BYTE and SharedLockGuard to include/myvector.h for broader access and cleaner architecture - Add #include <vector> to myvector.h (missing include) - Improve SharedLockGuard: rename clear() to release(), add deleted copy/move, make constructor explicit - Fix: VectorIndexCollection::open() now acquires shared lock before returning (matching get() behavior), preventing UB when SharedLockGuard releases a lock never acquired on the open() path - Fix: myvector_ann_set_init moves lock guard creation after null check to avoid constructing guard with null index - Fix: improve ER_MYVECTOR_INDEX_NOT_FOUND message to include index name - Fix: myvector_display returns string literal instead of writing to UDF result buffer (fixes invalid memory access) - Fix: format string %d -> %zu for size_t colinfo.length() Co-authored-by: askdba <5901296+askdba@users.noreply.github.com>
…atibility The PR #76 introduces a macro to abstract UDF metadata service access, preparing for component builds where the service is accessed differently. This aligns with the component migration plan in docs/COMPONENT_MIGRATION_PLAN.md. Co-authored-by: askdba <5901296+askdba@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Review open pull request for changes
Fix concurrency bugs and UDF correctness issues found in PR #76 review
Feb 25, 2026
Owner
|
Closing: bug fixes incorporated into feature/component-migration-8.4-9.6 branch. That branch has component migration work and will be merged to main, making this PR redundant and conflicted. |
askdba
added a commit
that referenced
this pull request
Feb 25, 2026
- SharedLockGuard: make release-only (no acquire in constructor) to avoid double-lock when used with get() which already acquires before returning - VectorIndexCollection::open(): acquire lockShared() before returning to match get() behavior; SharedLockGuard expects caller to hold lock - Keeps component build intact (PR #78 targeted main without component) Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review of PR #76 (component migration 8.4–9.6) uncovered several pre-existing bugs in the plugin code. This PR applies those fixes and a few structural cleanups to the main plugin sources.
Bug Fixes
VectorIndexCollection::open()missing shared lock —get()acquireslockShared()before returning;open()did not, soSharedLockGuardwould callunlockShared()on a never-locked mutex (UB) in any code path that creates a new index viaopen()then drops or scopes out normally.myvector_ann_set_init: null-before-lock ordering —SharedLockGuard l(vi)was constructed before theif (!vi)null check, passing a potentially-null pointer to the guard. Reordered to check null first. Also improves the error message to include the missing index name.myvector_display: returns string literal from UDF —return "<invalid vector>"on checksum failure returned a pointer to a read-only string literal instead of writing to theinitid->ptrresult buffer, causing incorrect behavior or crashes in MySQL's UDF return path.Format string mismatch —
%dused forcolinfo.length()(size_t) → UB on LP64 platforms. Fixed to%zu.Structural Changes
SharedLockGuardmoved toinclude/myvector.h— required for upcoming component build sources. Added deleted copy/move, explicit constructor, renamedclear()→release().BITS_PER_BYTEmoved to header — wasstatic constlocal tomyvector.cc; promoted toconstexprin the shared header.MYVECTOR_UDF_METADATA()macro — abstracts the UDF metadata service accessor; needed for the component build path in PR Component migration (8.4–9.6) and release workflow update #76 wheremyvector_component_udf_metadatareplaces(*h_udf_metadata_service).Design Note on
SharedLockGuardPR #76 proposed having
SharedLockGuardacquirelockShared()in its constructor. This was not applied here:get()already acquires the shared lock before returning (to close the race window whilem_mutexis held), so adding a secondlockShared()in the guard constructor would double-acquirestd::shared_mutexfrom the same thread — UB per C++17. The correct fix is ensuringopen()also acquires the lock (done here), keeping the guard as release-only.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.