deepthi912 opened a new pull request, #19596: URL: https://github.com/apache/pinot/pull/19596
## Summary The upsert metadata manager caches `IndexSegment` references inside `RecordLocation` entries. Between when a reference is cached and when it is later dereferenced (during revert, partial-upsert merge, addRecord, addSegment iteration, doRemoveExpiredPrimaryKeys), the segment can be offloaded and destroyed by the `TableDataManager`, leaving the cached reference pointing at freed native buffers. Any subsequent read of `getValidDocIds`, `getQueryableDocIds`, column readers, or `LazyRow` columns then hits a destroyed segment and can SIGSEGV. This PR closes that race by having the upsert metadata paths participate in the existing `SegmentDataManager` refcount protocol before dereferencing any cached segment reference. ## Changes **`UpsertContext#acquireIfSame(IndexSegment)`** — new helper that atomically bumps the owning SDM's refcount and verifies it still wraps the expected segment (catches the same-name-different-instance replace race). While the returned handle is held, `IndexSegment#destroy()` cannot run, so all subsequent dereferences of the cached segment are safe. Release via `UpsertContext#releaseSegment` in a `finally`. **`ConcurrentMapPartitionUpsertMetadataManager`** and **`ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes`** — every dereference of a cached segment's native buffers is now bracketed by acquire/release: | Method | What's protected | |---|---| | `doAddOrReplaceSegment` (different-segment branch) | `replaceDocId(segment, ..., currentSegment, ...)` | | `revertAndRemoveSegment` | `prevSegment.getValidDocIds`, `RecordInfoReader(prevSegment, ...)`, `replaceDocId(prevSegment, ...)` | | `doRemoveExpiredPrimaryKeys` | `getQueryableDocIds`, `removeDocId(segment, docId)` | | `doAddRecord` (different-segment branch) | `replaceDocId(segment, ..., currentSegment, ...)` | | `doUpdateRecord` (partial-upsert merge) | `LazyRow.init(currentSegment, ...)` and column reads inside `PartialUpsertHandler.merge` | Consuming (`MutableSegment`) currents are held alive by consumer ownership and skip the acquire. ## What is NOT changed - Method-parameter segments (`segment`, `oldSegment`) — caller (offload / add flow) holds the ref for the duration of the call. - Identity checks (`==`), `instanceof` checks, `getSegmentName()` (String field) — none of these touch native buffers. - Query-path acquires — already handled by `SingleTableExecutionInfo` / `TableDataManager.acquireSegments`. ## Test plan - [ ] Existing upsert integration tests pass - [ ] Add a targeted concurrency test that races segment offload with `revertAndRemoveSegment` / `doUpdateRecord` and asserts no SIGSEGV / no unhandled exception (follow-up) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
