fix: keep the by-row-number decode's scratch out of the fetch cache (#353) - #357
Conversation
…datcmd#353) An indexed point query on a wide table at the default stripe_row_limit ran at ~33 ms PER ROW (186x slower than one stripe size down, ~1700x vs heap): the by-row-number fetch cache dropped the decoded row group on every fetch and re-decoded it, because the entry exceeded COLUMNAR_FETCH_CACHE_MAX_BYTES (32 MB). The measured cause was not the decoded result but the decode's SCRATCH. columnar_native_decode_chunk allocates its intermediates -- the decompressed encoded region (ColumnarDecompressValueStream) and each vector's decoded buffer (ColumnarDecodeChunk, memcpy'd into rawBuf and then never freed) -- in the caller's context. On the scan path that context is a group context reset every group, so it never mattered. On the by-row-number fetch path the context is the statement-scoped cache entry, which is NOT reset, so the scratch stayed live and roughly tripled the entry: a 13-column projected prefix whose tight decoded size was ~11 MB measured ~34 MB, over the cap, so every fetch re-decoded the whole group. (Instrumented: entry->cx 33.8 MB of which raw chunk bytes were 0.25 MB and decoded+scratch 33.6 MB; the same with ALLOCSET_SMALL_SIZES, so it was live scratch, not aset block reservation.) Put the decode's scratch in a transient child context and delete it before returning, so the caller's context keeps only rawBuf and vecRawLen. The default- stripe wide-table point query drops from ~19,800 ms to ~98 ms (~200x), identical result. The scan path is unaffected (its context was already reset per group). This does not raise the cap or change the default stripe: it removes the inflation rather than moving the cliff. A wider projected prefix (or an allColumns fetch) whose tight decoded size genuinely exceeds 32 MB still degrades gracefully as before, so a cap GUC remains a possible follow-up if that shape shows up. test/native_fetch_cache.sh gains a wide-table point query asserting it is not far dearer than the small-group control (a skippable timing ratio: ~2x now vs ~200x before) and returns the same answer. 9/9 on PG18 assert; the fetch-path suites (fetch_cache/position/projection, native_index/ios/dml, alter_column_type) pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
|
Verified against my own #353 reproduction, and it holds. Also: your root cause is right and the one I put in #353 was wrong. Correcting that below, since my recommendation there would have sent someone down a path you have already shown cannot work. VerifiedSame script as #353, same three group sizes, same 300,000-row 21-column fixture, run against this branch:
At the default: 19,788 ms to 121 ms for the same 600 rows, 164x, results identical. The part that matters more than the ratio: the cliff is gone, not moved. Before, 150,000 was 186x worse than 100,000. Now the three sizes are 0.201 / 0.140 / 0.119, a smooth gradient reflecting group count rather than a threshold. That is what distinguishes this from raising the cap or lowering the default. Suites on PG15, PG17 and PG19: Correcting my analysis in #353I wrote that "a 150,000-row group of a 21-column table decodes to more than 32 MB" and reasoned from there. That was an inference from the symptom, and it was wrong: the decoded result is about 11 MB. The entry was inflated to ~34 MB by decode scratch left live in a context that is reset on the scan path and not on the fetch path. The consequence of my being wrong is not academic. I recommended per-column caching as "the only option that removes the cliff rather than moving it". You have shown it would not have worked, because the deferred index fetch decodes a prefix rather than the touched columns, so the entry would not drop below the cap either. Anyone who took my recommendation would have built something substantial and still had the bug. What I got right was the measurement: the 186x cliff, the flat ~28.7 ms per row across a twelve-fold change in row count, and the location. What I got wrong was inferring the mechanism from those numbers instead of instrumenting the allocation, which is what you did. I will amend #353 so the wrong recommendation is not left standing. On the fix itselfNineteen lines, one function, and it removes the inflation rather than accommodating it. The scan path is genuinely unaffected because its context was already reset per group, which is also why this went unnoticed for so long: the bug is invisible from the scan path and only the fetch path keeps the context alive. Ruling out the alternatives with reasons in the PR body, rather than just asserting the chosen one, is what makes this reviewable. The No objections. Merging. |
|
Correction to my review above. I said this removes the cliff rather than relocating it. That is not right, and I have filed the remainder as #359. Measured on merged main, same rows and same My verification used a narrow projection ( None of this diminishes the fix: 63x on q2 at 100M, the diagnosis was better than mine, and removing transient scratch from a cached allocation is right regardless. The remaining problem is that exceeding the cap degrades ~47x with nothing in between, which #359 covers. |
2 participants
What
Fixes #353: an indexed point query on a wide table at the default
stripe_row_limitran at ~33 ms per row (186× slower than one stripe size down, ~1700× vs heap) — the by-row-number fetch cache dropped the decoded group on every fetch and re-decoded it, because the entry exceededCOLUMNAR_FETCH_CACHE_MAX_BYTES(32 MB).Root cause (measured, not assumed)
It was not the decoded result and not the whole-group buffer — it was the decode's scratch.
columnar_native_decode_chunkallocates its intermediates (the decompressed encoded region, and each vector's decoded buffer that'smemcpy'd intorawBufthen never freed) in the caller's context. On the scan path that context is a group context reset every group, so it never mattered. On the fetch path it's the statement-scoped cache entry, which is not reset — so the scratch stayed live and roughly tripled the entry: a 13-column projected prefix whose tight decoded size was ~11 MB measured ~34 MB, over the cap.Instrumented:
entry->cx33.8 MB, of which raw chunk bytes were 0.25 MB and decoded+scratch 33.6 MB; unchanged underALLOCSET_SMALL_SIZES, so it was live scratch, not aset block reservation. (Ruled out along the way: per-column caching — the deferred index fetch decodes a prefix, not the touched columns, so it doesn't shrink below the cap; narrowing to projected columns — infeasible, the standard tableam + deferred slot only knows the prefix; raising the cap / lowering the stripe — moves the cliff rather than removing it.)Fix
Put the decode's scratch in a transient child context and delete it before returning, so the caller's context keeps only
rawBufandvecRawLen. 19 lines, one function. The scan path is unaffected (its context was already reset per group). This removes the inflation rather than raising the cap or lowering the default stripe.Default-stripe wide-table indexed point query: ~19,800 ms → ~98 ms (~200×), identical result.
Tests / gate
test/native_fetch_cache.shgains a wide-table point query asserting it's not far dearer than the small-group control (a skippable timing ratio: ~2× now vs ~200× before) and returns the same answer. Gate: preflight all 5 majors, full assert matrix PG18 + PG19, and pg18_san across the decode/fetch path (native_fetch_cache/projection, native_index, native_agg, native_dml) — all clean (only red is the pre-existinganalyze_statswall-clock flake, which fails identically on clean main).Note: this addresses the fetch cost half; the planner cost-modelling gap (#355) — the planner picking an index scan for ordering without pricing the per-row fetch — is separate and still open, though this makes it far less severe.
🤖 Generated with Claude Code