Hold what fits in the fetch cache instead of dropping it all (#359)

design/ISSUE_359_FETCH_CACHE_PARTIAL.md

@@ -0,0 +1,126 @@
+# #359: make fetch-cache overflow proportional instead of a cliff
+
+Status: design, 2026-08-03. Successor to #353/#357. Coupled to #355 (merged as #360).
+
+## The defect
+
+`columnar_fetch_row` (`src/columnar_reader.c`) ends every fetch with:
+
+```c
+if (MemoryContextMemAllocated(entry->cx, true) > COLUMNAR_FETCH_CACHE_MAX_BYTES)
+    columnar_fetch_entry_reset(entry);
+```
+
+That is all-or-nothing. An entry one byte over the 32 MB cap is not retained at
+all, so the next fetch into the same group re-reads the group's bytes from disk
+**and** re-decodes every column it touches. Measured on the 100M TSBS fixture,
+holding rows and plan shape constant and varying only the number of aggregated
+columns: 4 columns 2,833 ms, 5 columns 134,147 ms. A 47x step, flat either side.
+
+#357 shrank entries ~3x by moving decode scratch out. That moved the threshold
+from "any wide table" to "five or more aggregate columns". It did not change the
+shape of the failure, and raising the cap would not either.
+
+## What the entry already gets right
+
+Decoding is **already per column and lazy**: `entry->rawBuf[c]` is filled only
+when column `c` is actually touched, and a column outside the projection is
+neither read nor decoded. The cache is therefore already column-granular on the
+way *in*. It is only the eviction that is whole-entry.
+
+That is the whole fix: make eviction as granular as admission already is.
+
+## Design: stable per-column admission
+
+Keep, always:
+
+- `groupBuffer` — the group's raw bytes. This is the on-disk (encoded,
+  compressed) form, and retaining it alone removes the per-fetch `ColumnarReadLogicalData`
+  disk read even when no column stays resident.
+- `rankPrefix[c]` and `valOffset[c]` — the position indexes from #143. These are
+  small relative to the decoded stream (`rankPrefix` is 4 bytes per 64 rows;
+  `valOffset` is 4 bytes per value against a value that is typically wider), and
+  they remain **valid for a re-decode**, because decoding the same chunk bytes is
+  deterministic and produces the same layout. Keeping them is what stops #143's
+  quadratic from coming back through the overflow path.
+
+Evict, per column:
+
+- After a column is decoded *and its value has been extracted*, if the entry is
+  over the cap, delete just that column's decoded stream and mark the column
+  overflowed. From then on that column decodes into the per-fetch scratch
+  context (`tmp`) and is freed with it.
+
+Admission order is attribute order, which is deterministic, so the resident set
+is **stable**: the same columns stay resident on every fetch into that group.
+
+### Why the resident set must be stable, not LRU
+
+This is the load-bearing decision. An LRU eviction *within* the entry is the
+obvious design and it is wrong here. The access pattern is cyclic: each fetch
+touches columns 0..n-1 in order. LRU against a cyclic access pattern whose
+working set exceeds the cache evicts precisely the entry about to be needed —
+
+- fetch 1 decodes 0,1,2,3,4; over cap, LRU evicts 0
+- fetch 2 wants 0: miss, decode; over cap, LRU evicts 1
+- fetch 2 wants 1: miss, decode; over cap, LRU evicts 2 ...
+
+— giving a 100% miss rate, i.e. exactly today's behaviour with more bookkeeping.
+A stable resident set of `k` of `n` columns re-decodes `n - k` per fetch. That is
+the proportionality the issue asks for, and it is why "retain what fits" has to
+mean *first-fit, then stop*, not *keep the hottest*.
+
+### Memory bound is preserved
+
+Today the entry transiently exceeds the cap (it is measured after the fetch that
+filled it) and is then dropped. Under this design the entry transiently exceeds
+the cap by one column's decoded stream and is then trimmed back under it, so
+steady-state residency is still <= cap per entry, 4 entries.
+
+One new case: if `groupBuffer` *alone* exceeds the cap, every column overflows
+and the entry would hold an unbounded raw buffer. Guard that explicitly — do not
+cache the entry at all in that case, which is exactly today's behaviour and keeps
+the bound at 4 x cap.
+
+### Two details that will bite
+
+1. **Baseline encoding aliases the group buffer.** When the encoding descriptor
+   is `COLUMNAR_NATIVE_ENCDESC_BASELINE`, `rawBuf[c] = base + validityBytes` —
+   a pointer *into* `groupBuffer`, not an allocation. It costs nothing to retain
+   and must never be "evicted" (that would be a free of an interior pointer).
+   Only a decoded stream is evictable.
+
+2. **The value must be extracted before the eviction.** `ColumnarDecodeValue`
+   copies into the caller's context for every case (by-value returns the datum;
+   fixed-length and varlena both `MemoryContextAlloc(targetContext)` + `memcpy`),
+   so freeing the column's stream after extraction is safe — but only after.
+
+## Consistency with #355
+
+`columnar_index_fetch_penalty` (`src/columnar_customscan.c:638`) currently has:
+
+```c
+/* #359 cliff: a group too wide to cache is re-decoded on every fetch */
+if ((double) rel->reltarget->width * R > (double) COLUMNAR_FETCH_CACHE_MAX_BYTES)
+    groups_decoded = groups_max;
+```
+
+That branch models the cliff this change removes. It must soften to the overflow
+fraction: when the decoded group exceeds the cap, the share of the decode that
+repeats per fetch is the share of the projection that does not fit, so the
+penalty scales by that fraction rather than jumping to "every group re-decoded".
+
+## Test
+
+`test/native_fetch_cache.sh`'s #353 case measures `count(*), max(u)` — a
+two-column projection — across two group sizes. That shape never crosses the
+relocated cap, which is why it went green on the query family that still cliffs.
+Both the issue author and the reviewer generalised from a single projection
+width; the test has to vary the axis that was held constant.
+
+Add a case that **varies projection width** at fixed group size, and assert the
+absence of a step rather than an absolute time: crossing the cap must cost
+proportionally more, not multiples more.
+
+Per `prove-guards-by-removal`: the new check must be shown to fail with this
+change reverted, on the same container, before it counts.

src/columnar_customscan.c

@@ -609,6 +609,7 @@ columnar_index_fetch_penalty(RelOptInfo *rel, double rows, double rho,
 	double		groups_min,
 				groups_max,
 				groups_decoded,
+				decoded_width,
 				csq;
 
 	if (rows <= 0 || R < 1)
@@ -635,9 +636,25 @@ columnar_index_fetch_penalty(RelOptInfo *rel, double rows, double rho,
 		groups_decoded = groups_min + (groups_max - groups_min) * (1.0 - csq);
 	}
 
-	/* #359 cliff: a group too wide to cache is re-decoded on every fetch */
-	if ((double) rel->reltarget->width * R > (double) COLUMNAR_FETCH_CACHE_MAX_BYTES)
-		groups_decoded = groups_max;
+	/*
+	 * A group too wide to hold entirely in the fetch cache re-decodes the part
+	 * that did not fit, once per fetch rather than once per group.
+	 *
+	 * This was a cliff -- the whole entry was dropped, so exceeding the cap by
+	 * any margin meant every group re-decoded, and the model said so. #359 made
+	 * the cache admit columns until the cap and re-decode only the remainder, so
+	 * the extra decoding is now the overflow *fraction* of the projection. Model
+	 * it the same way: blend between decoding each group once and decoding one
+	 * per fetch, by how much of the decoded group does not fit.
+	 */
+	decoded_width = (double) rel->reltarget->width * R;
+	if (decoded_width > (double) COLUMNAR_FETCH_CACHE_MAX_BYTES)
+	{
+		double		resident = (double) COLUMNAR_FETCH_CACHE_MAX_BYTES /
+			decoded_width;
+
+		groups_decoded += (groups_max - groups_decoded) * (1.0 - resident);
+	}
 
 	if (groups_decoded < 0)
 		groups_decoded = 0;

src/columnar_reader.c

@@ -1793,6 +1793,15 @@ ColumnarFreeLivenessCache(ColumnarLivenessCache *cache)
  * sizing from disk would overshoot worst on exactly the tables this helps most.
  * COLUMNAR_FETCH_CACHE_MAX_BYTES is defined in columnar.h so the index-fetch cost
  * model (#355) can name the same cap.
+ *
+ * The cap is enforced per column, not per entry (issue #359). It used to drop the
+ * whole entry, which made it a cliff: an entry one byte over was not retained at
+ * all, so every fetch re-read the group and re-decoded every column it touched.
+ * On the 100M fixture that was 2,833 ms at four aggregate columns and 134,147 at
+ * five, flat either side -- a 47x step inside the space of ordinary queries.
+ * Shrinking entries (#357 moved decode scratch out, ~3x) only moved the step.
+ * Now the columns that fit stay resident and only the remainder is re-decoded, so
+ * crossing the cap costs the overflow fraction rather than everything.
  */
 
 typedef struct ColumnarFetchGroup
@@ -1824,6 +1833,27 @@ typedef struct ColumnarFetchGroup
 	 */
 	uint32	  **rankPrefix;		/* [natts]; NULL until that column is decoded */
 	uint32	  **valOffset;		/* [natts]; NULL for fixed-length columns */
+
+	/*
+	 * Per-column residency, so exceeding the cap costs proportionally rather
+	 * than totally (issue #359).
+	 *
+	 * colCx[c] holds column c's decoded stream, one child context per column so
+	 * a single column can be released without disturbing the rest of the entry.
+	 * It is NULL when the column is undecoded, and also when the column decodes
+	 * to a pointer into groupBuffer (baseline encoding allocates nothing).
+	 *
+	 * overflow[c] marks a column that was decoded, did not fit, and was
+	 * released. Such a column decodes into per-fetch scratch from then on. The
+	 * mark is never cleared: the resident set has to be *stable*, because the
+	 * access pattern here is cyclic -- every fetch touches the same projected
+	 * columns in attribute order -- and evicting the least recently used column
+	 * against a cyclic pattern evicts precisely the column about to be needed,
+	 * which is a 100% miss rate, i.e. the very behaviour this removes. First
+	 * fit and then stop re-decodes only the columns that did not fit.
+	 */
+	MemoryContext *colCx;		/* [natts] */
+	bool	   *overflow;		/* [natts] */
 	uint64		lastUsed;
 }			ColumnarFetchGroup;
 
@@ -2230,6 +2260,8 @@ columnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 		entry->rawBuf = palloc0(sizeof(char *) * natts);
 		entry->rankPrefix = palloc0(sizeof(uint32 *) * natts);
 		entry->valOffset = palloc0(sizeof(uint32 *) * natts);
+		entry->colCx = palloc0(sizeof(MemoryContext) * natts);
+		entry->overflow = palloc0(sizeof(bool) * natts);
 		MemoryContextSwitchTo(tmp);
 
 		if (rg->byteLength > 0)
@@ -2288,6 +2320,7 @@ columnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 		char	   *rawBuf;
 		char	   *cursor;
 		uint64		present;
+		bool		justDecoded;	/* this fetch decoded it; may not fit */
 
 		/*
 		 * A column the caller did not ask for is neither decoded nor indexed,
@@ -2317,43 +2350,87 @@ columnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 			continue;
 		}
 
-		if (entry->rawBuf[c] == NULL)
+		if (entry->rawBuf[c] != NULL)
+		{
+			rawBuf = entry->rawBuf[c];
+			justDecoded = false;
+		}
+		else
 		{
-			MemoryContext decOld = MemoryContextSwitchTo(entry->cx);
+			/*
+			 * A baseline chunk is not encoded, so its "decoded" stream is a
+			 * pointer into groupBuffer rather than an allocation. It costs the
+			 * entry nothing beyond the group bytes it already holds, so it is
+			 * always resident and is never a candidate for release below --
+			 * releasing it would mean freeing an interior pointer.
+			 */
+			bool		baseline = (cc->encodingDescriptorLen == 1 &&
+									(uint8) cc->encodingDescriptor[0] ==
+									COLUMNAR_NATIVE_ENCDESC_BASELINE);
+			MemoryContext decCx;
+			MemoryContext decOld;
+
+			if (baseline)
+				decCx = entry->cx;
+			else if (entry->overflow[c])
+				decCx = tmp;	/* known not to fit: decode per fetch */
+			else
+				decCx = AllocSetContextCreate(entry->cx,
+											  "columnar fetch column",
+											  ALLOCSET_DEFAULT_SIZES);
 
-			if (cc->encodingDescriptorLen == 1 &&
-				(uint8) cc->encodingDescriptor[0] == COLUMNAR_NATIVE_ENCDESC_BASELINE)
-				entry->rawBuf[c] = base + validityBytes;
+			decOld = MemoryContextSwitchTo(decCx);
+			if (baseline)
+				rawBuf = base + validityBytes;
 			else
-				entry->rawBuf[c] =
-					columnar_native_decode_chunk(entry->cx, att,
+				rawBuf =
+					columnar_native_decode_chunk(decCx, att,
 												 base + validityBytes,
 												 (uint32) (cc->pageLength - validityBytes),
 												 cc->encodingDescriptor,
 												 cc->encodingDescriptorLen,
 												 cc->blockCodec, NULL, NULL);
+			MemoryContextSwitchTo(decOld);
 
 			/*
 			 * Index the column while it is being decoded, so every fetch into
-			 * this group afterwards reaches its row directly (issue #143). Both
-			 * indexes live in the entry context and so are measured by the cap
-			 * below and dropped with the rest of the entry.
+			 * this group afterwards reaches its row directly (issue #143).
+			 *
+			 * The indexes live in the entry context, not in the column's, so
+			 * they outlive a released column. That is deliberate: decoding the
+			 * same chunk bytes is deterministic and yields the same layout, so
+			 * the offsets stay valid across a re-decode. An overflowed column
+			 * therefore pays its decode again but still reaches its row in
+			 * constant time, which is what stops #143's quadratic returning
+			 * through this path. They are small next to the stream: rankPrefix
+			 * is four bytes per 64 rows, valOffset four per value.
 			 */
-			entry->rankPrefix[c] = columnar_build_rank_prefix(vbits,
-															 entry->rowCount);
-			if (att->attlen < 0)
+			if (entry->rankPrefix[c] == NULL)
 			{
-				uint64		nblocks = (entry->rowCount +
-									   COLUMNAR_RANK_BLOCK_ROWS - 1) /
-					COLUMNAR_RANK_BLOCK_ROWS;
+				MemoryContext idxOld = MemoryContextSwitchTo(entry->cx);
+
+				entry->rankPrefix[c] = columnar_build_rank_prefix(vbits,
+																 entry->rowCount);
+				if (att->attlen < 0)
+				{
+					uint64		nblocks = (entry->rowCount +
+										   COLUMNAR_RANK_BLOCK_ROWS - 1) /
+						COLUMNAR_RANK_BLOCK_ROWS;
 
-				entry->valOffset[c] =
-					columnar_build_val_offsets(att, entry->rawBuf[c],
-											   entry->rankPrefix[c][nblocks]);
+					entry->valOffset[c] =
+						columnar_build_val_offsets(att, rawBuf,
+												   entry->rankPrefix[c][nblocks]);
+				}
+				MemoryContextSwitchTo(idxOld);
 			}
-			MemoryContextSwitchTo(decOld);
+
+			if (decCx != tmp)
+			{
+				entry->rawBuf[c] = rawBuf;
+				entry->colCx[c] = baseline ? NULL : decCx;
+			}
+			justDecoded = true;
 		}
-		rawBuf = entry->rawBuf[c];
 
 		/*
 		 * The row's value sits at the rank-th position in the present-value
@@ -2371,20 +2448,45 @@ columnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 
 		values[c] = ColumnarDecodeValue(att, &cursor, target);
 		nulls[c] = false;
+
+		/*
+		 * Release this column if it is the one that took the entry over the cap
+		 * (issue #359). Measuring the context is what makes the cap mean decoded
+		 * bytes rather than stored bytes; a column of wide text decodes to many
+		 * times its size on disk, and sizing from disk would overshoot worst on
+		 * exactly the tables this helps most.
+		 *
+		 * Releasing here rather than dropping the whole entry is what makes
+		 * going over the cap cost proportionally: the columns admitted before
+		 * this one stay resident and are decoded once, and only the remainder is
+		 * decoded per fetch. groupBuffer stays either way, so no fetch re-reads
+		 * the group from disk.
+		 *
+		 * It is safe because nothing handed back points into the column: the
+		 * value returned was copied into the caller's context by the
+		 * ColumnarDecodeValue call immediately above, and the position indexes
+		 * live in entry->cx rather than in the column's own context.
+		 */
+		if (justDecoded && entry->colCx[c] != NULL &&
+			MemoryContextMemAllocated(entry->cx, true) >
+			COLUMNAR_FETCH_CACHE_MAX_BYTES)
+		{
+			MemoryContextDelete(entry->colCx[c]);
+			entry->colCx[c] = NULL;
+			entry->rawBuf[c] = NULL;
+			entry->overflow[c] = true;
+		}
 	}
 
 	/*
-	 * Hold the entry only while it is worth holding. Measuring the context is
-	 * what makes the cap mean decoded bytes rather than stored bytes; a group
-	 * over the cap is used for this fetch and then dropped, so an outsized group
-	 * costs what it always did rather than pinning memory.
-	 *
-	 * Dropping it here is safe because nothing handed back points into it: the
-	 * value that is returned is decoded into the caller's context by the
-	 * ColumnarDecodeValue call above. Only the decoded stream and its two
-	 * position indexes live in entry->cx.
+	 * The per-column release above trims the entry back under the cap, so the
+	 * entry is bounded by it -- except in one case it cannot help: a group whose
+	 * *raw* bytes alone exceed the cap. Every column would overflow and the entry
+	 * would still pin groupBuffer, so four such entries could hold arbitrarily
+	 * much. Drop the entry whole there, which is what this path did for every
+	 * oversized group before, and keeps the cache bounded by 4 x the cap.
 	 */
-	if (MemoryContextMemAllocated(entry->cx, true) > COLUMNAR_FETCH_CACHE_MAX_BYTES)
+	if (rg->byteLength > COLUMNAR_FETCH_CACHE_MAX_BYTES)
 		columnar_fetch_entry_reset(entry);
 
 	MemoryContextSwitchTo(oldContext);