pcache: address !127 review follow-ups (Stats accuracy + EasyRefusals counter)

Ian Chechinrequested to merge
Deln0r/sqlite:pcache-pool-polish into master

Motivation

The !127 (merged) merge note flagged three non-blocking follow-ups for a later MR. This MR collects all three into a single polish pass and asks one open question on how to take the I/O comparison further.

Quote from your !127 (merged) LGTM:

A few non-blocking follow-ups (nothing here gates the merge - fine for a later MR):

  1. FetchCreateEasy refuses at cap even when there are recyclable unpinned pages, where pcache1 would recycle one without spilling. Correct, but it likely spills more under pressure - might be worth an I/O comparison vs pcache1 alongside the memory numbers.
  2. Stats.Evictions also counts discard-unpins and Shrink releases (not just LRU), while Truncate/Rekey/Destroy frees aren't counted
    • the field doc reads narrower than the behavior.
  3. The TestPoolRoundTripIntegrity comment mentions exercising xRekey ~15 times, but the run reports Rekeys:0 - the SQL surface doesn't hit it here; rekey is covered by the unit tests, as the bench comment already notes.

What this MR delivers

Item 2: Stats.Evictions documentation

The field doc is tightened to match the actual behavior. The new docstring states explicitly that Evictions counts LRU eviction, `Unpin(discard=true)`, and `Shrink` releases; bulk frees performed by `Truncate`, `Rekey` collisions, and `Destroy` are not counted. The old "LRU-driven page releases" wording read narrower than the implementation.

Item 1: Stats.EasyRefusals counter + benchmark surface

New `Stats.EasyRefusals` counter. Each time `FetchCreateEasy` refuses at cap, the counter increments. SQLite reacts to a refusal by spilling dirty pages and retrying with `FetchCreateForce`, so EasyRefusals/op is a direct in-package proxy for the I/O pressure the strict Easy contract imposes vs pcache1's recycle-without-spill behavior.

Both existing benchmarks (`BenchmarkPoolBoundedCache` and `BenchmarkPoolEvictionChurn`) now report `easy-refusals/op` alongside the existing page-allocs/evictions metrics. `TestEasyHonoursCacheSize` was extended to assert the counter increments on each Easy refusal.

Sample benchmark output on darwin/arm64 confirms the metric surfaces under pressure (`cache_size=16`, DELETE + incremental_vacuum + INSERT cycle):

``` BenchmarkPoolEvictionChurn-8 50 800400 ns/op 2.680 easy-refusals/op 133.0 page-allocs/op 133.0 page-evictions/op ```

The 1:1 allocs:evictions ratio is the steady-state strict-bounding signal we already had; the new 2.68 easy-refusals/op tells the spill-pressure story the strict contract adds on top of pcache1.

Item 3: TestPoolRoundTripIntegrity comment

The previous comment claimed the DELETE + incremental_vacuum workload exercised xRekey "~15 times on cznic's harness." The actual run reports `Rekeys: 0` on every platform we tested. The corrected comment notes that the SQL surface here does not reliably emit xRekey and that the code path is covered by the unit tests (`TestRekey`, `TestRekeyEvictsCollider`) instead.

Open question — please confirm direction

For the deeper "I/O comparison vs pcache1" angle you raised in item 1: a direct side-by-side against the in-engine pcache1 would require either

(a) exposing `sqlite3_db_status` through the parent driver so the benchmark can read `SQLITE_DBSTATUS_CACHE_HIT`, `CACHE_MISS`, `CACHE_SPILL`, `CACHE_WRITE` deltas per connection, or (b) running two separate test binaries (one with `sqlite.MustRegisterPageCache(pcache.New())` in TestMain, one without) and comparing the parent-driver numbers offline.

Did I read the original ask correctly, and which of (a) or (b) is your preferred next step? I am holding the deeper comparison until direction is settled rather than guess and rework.

For now, the in-package `EasyRefusals/op` proxy is the closest useful signal we can surface from inside the pcache subpackage without touching the parent driver's exported surface.

Verification

  • `go test -race -short -count=1 ./pcache/` clean on darwin/arm64 (16 unit + 2 e2e, including the new `EasyRefusals` assertions).
  • Cross-build clean for linux/amd64, linux/arm64, linux/386, darwin/amd64, darwin/arm64, windows/amd64, freebsd/amd64, openbsd/amd64.
  • `gofmt -l pcache/` clean.
  • `go vet ./pcache/` introduces no new warnings beyond the two pre-existing `unsafe.Pointer` notices on `Buf`/`Extra`.

What this MR explicitly does NOT do

  • No behavioral change to FetchCreateEasy. The Easy contract still refuses at cap; the new EasyRefusals counter only observes the refusal rate.
  • No new public API beyond the `Stats.EasyRefusals` field. The existing `Stats` shape is otherwise unchanged.
  • No pcache1 side-by-side numbers yet — pending direction on (a) vs (b) above.

Merge request reports