pcache: per-cache mutex for -race cleanliness under cache=shared

Ian Chechinrequested to merge
Deln0r/sqlite:pcache-shared-cache-draft into master

Status: DRAFT / RFC — no behavior change

This MR is a scaffolding commit asking for direction before any concurrency primitive, wrapper type, or PageCache contract change is written. The single file (`pcache/sharing.go`) compiles and changes no runtime behavior; existing tests stay green. The purpose is to get directional answers on three design questions in a focused diff before the real implementation lands as a follow-up.

Motivation

From the !127 (merged) merge note:

The unsynchronized per-Cache state is fine under the current FULLMUTEX + no-shared-cache + database/sql invariant — that's the assumption MR-C will need to revisit.

Adding cross-connection / shared-cache support breaks that invariant: when the engine reuses one `pCache*` across multiple Btree instances opened from the same shared-cache scope, calls into our `Cache` may overlap. The current `pcache` impl would race in that case. This MR opens the design discussion before the implementation commits to one shape.

Three open design questions

The full reasoning is in `pcache/sharing.go` so the questions live next to the code they constrain. Summary:

Q1 — Concurrency primitive. `sync.Mutex` over every Cache method is the simplest correct shape. `sync.RWMutex` buys little because `Fetch` is read-modify (touches LRU position or allocates on miss). Finer-grained per-bucket locking does not pay off either because every callback runs short. Current lean: `sync.Mutex` on the cache struct. Open to a different direction.

Q2 — Locking surface. Three plausible shapes:

(a) lock lives on the existing `cache` struct in `pool.go`, always taken. Zero behavioral difference for non-shared callers, but the uncontended-lock cost is paid by every connection regardless of mode.

(b) a `sharedCache` wrapper type that adds the lock and is returned from `Pool.Create` only when the engine asks for a shared cache. Cheaper for the non-shared common case but requires the binding to forward a shared-cache signal to the PageCache, which `PageCache.Create` does not expose today.

(c) make `sqlite.Cache` optionally implement a concurrency hint (`ConcurrentSafe() bool`) and let the binding wrap when an impl returns false. Pushes the choice down to the impl.

Q3 — Discovery. `PageCache.Create` does not currently receive an indicator of shared-cache scope. Three signalling options:

(a) extend `PageCache.Create` with a new parameter. Breaks the `sqlite.PageCache` interface from !126 (merged).

(b) add a separate `PageCache.CreateShared` entry point. Backward-compatible but doubles the API surface.

(c) detect at the binding level (read the URI on the parent conn at `xCreate` time) and call a different impl method. Keeps the user-facing `PageCache` interface clean and confines the new code to the binding.

Test surface this MR is also asking about

The canonical e2e for shared-cache safety would be two connections to the same database with `cache=shared`, concurrent writes, `PRAGMA integrity_check` at the end. Easy to write once Q1/Q2/Q3 are settled. The test name is reserved in the scaffold; no test code is added yet because the locking shape decides whether it belongs on `cache`, on a `sharedCache` wrapper, or on the binding.

What this MR explicitly does NOT do

  • No `sync.Mutex` or other primitive added anywhere yet.
  • No `PageCache` interface change.
  • No new exported types or methods. The `sharedCacheStub` type is a placeholder kept so the file is non-empty and the docs render in godoc; it has no behavior and is removed in the follow-up that implements Q1/Q2/Q3.
  • No CHANGELOG entry — this is RFC, not a feature.
  • No binding-level changes in the parent package.

Verification

  • `go build ./...` clean.
  • `gofmt -l pcache/` clean.
  • `go vet ./pcache/` introduces no new warnings (the two pre-existing `unsafe.Pointer` notices on `Buf`/`Extra` are unaffected).
  • `go test -race -short -count=1 ./pcache/` ok (all existing tests pass; no new tests added).

Sequence after direction is settled

Once Q1/Q2/Q3 are answered, the same branch will receive one focused commit that

  • replaces or removes the `sharedCacheStub`,
  • adds the concurrency primitive per Q1 on the structure per Q2,
  • wires the shared-cache discovery signal per Q3,
  • adds `TestSharedCacheTwoConns_Integrity`,
  • and adds the CHANGELOG entry.

That second commit is what asks for review-and-merge. This one only asks for direction.

References !127 (merged) review note and the original !126 (merged) description's deferred-to-MR-C scope.

Merge request reports