Skip to content

tp: prove StringPool read path is concurrent-safe under locking - #7344

Open
LalitMaganti wants to merge 1 commit into
mainfrom
dev/lalitm/string-pool-concurrent-reads
Open

tp: prove StringPool read path is concurrent-safe under locking#7344
LalitMaganti wants to merge 1 commit into
mainfrom
dev/lalitm/string-pool-concurrent-reads

Conversation

@LalitMaganti

@LalitMaganti LalitMaganti commented Sep 4, 2026

Copy link
Copy Markdown
Member

Adds two ThreadSanitizer stress tests proving the StringPool read path
is safe for the multi-connection design, where string columns are read
on every query thread:

  • ConcurrentReadsNoInterns: 8 threads concurrently Get()/GetId() a
    finalized pool (small, block-spanning and >1MB large strings).
  • ConcurrentReadsWhileLockedInterns: with locking enabled, reader
    threads do lock-free Get() of pre-existing ids while writer threads
    continuously intern ~2KB strings, forcing several real 4MB-block
    allocations during the read window (the new-block publication path),
    plus occasional large-string appends. The test asserts the bytes of
    every pre-existing id stay intact and self-validates that block
    growth actually occurred.

No production fix was required: the lock-free Get() is safe against
concurrent interns because |blocks_| is a fixed array that never
reallocates (a new block writes a different slot), block bytes are
written forward-only before the Id is published, and large strings are
fully mutex-guarded. Documents this concurrency contract on
set_locking() — including that concurrent interns from multiple threads
are serialized by the mutex and supported (the prior draft contract
wrongly stated otherwise).

@LalitMaganti
LalitMaganti force-pushed the dev/lalitm/string-pool-concurrent-reads branch from 76e729a to 850aa56 Compare September 4, 2026 17:06
@LalitMaganti
LalitMaganti force-pushed the dev/lalitm/string-pool-concurrent-reads branch from 850aa56 to 59a29ab Compare September 4, 2026 17:06
@LalitMaganti
LalitMaganti marked this pull request as ready for review September 4, 2026 17:08
@LalitMaganti
LalitMaganti requested a review from a team as a code owner September 4, 2026 17:08
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Base automatically changed from dev/lalitm/dataframe-concurrent-cursors to main September 4, 2026 17:51
Adds two ThreadSanitizer stress tests proving the StringPool read path
is safe for the multi-connection design, where string columns are read
on every query thread:

 - ConcurrentReadsNoInterns: 8 threads concurrently Get()/GetId() a
   finalized pool (small, block-spanning and >1MB large strings).
 - ConcurrentReadsWhileLockedInterns: with locking enabled, reader
   threads do lock-free Get() of pre-existing ids while writer threads
   continuously intern ~2KB strings, forcing several real 4MB-block
   allocations during the read window (the new-block publication path),
   plus occasional large-string appends. The test asserts the bytes of
   every pre-existing id stay intact and self-validates that block
   growth actually occurred.

No production fix was required: the lock-free Get() is safe against
concurrent interns because |blocks_| is a fixed array that never
reallocates (a new block writes a different slot), block bytes are
written forward-only before the Id is published, and large strings are
fully mutex-guarded. Documents this concurrency contract on
set_locking() — including that concurrent interns from multiple threads
are serialized by the mutex and supported (the prior draft contract
wrongly stated otherwise).
@LalitMaganti
LalitMaganti force-pushed the dev/lalitm/string-pool-concurrent-reads branch from 59a29ab to b2d4fa9 Compare September 4, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants