From 05a3dd801d330a40ccf7ac039d7f49fe3565d851 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sat, 25 Jul 2026 18:02:35 -0600 Subject: [PATCH 1/5] Cache the decoded row group across fetches within a statement ColumnarReadRowByNumber() read and decoded a whole row group to return one row, so fetching N rows out of one group cost N times the group (issue #143). Every index scan, bitmap scan, and index-driven UPDATE or DELETE goes through it. This is option B of design/FETCH_BY_ROW_PLAN.md: keep the group's bytes and its decoded columns in a statement-scoped cache keyed by (storageId, groupNumber), filled on a miss and reused while the command that filled it is still running. Correctness comes from the scope rather than from an invalidation protocol. A row group's bytes are immutable once written; a rewrite or compaction needs a lock the reading statement already excludes; and a reclaim that reuses a file offset can only happen in a later command, which the command id check rejects. Keying on the storage id means new storage is never served an old decode. Visibility is not cached: the delete vector, the buffered delete marks and the validity bitmap are consulted per fetch, so a row deleted between two fetches in one statement is still seen as deleted. The row-group list is also read per fetch, deliberately, so a group flushed earlier in the same statement becomes visible. Entries live in contexts under TopTransactionContext, so commit or abort frees them; the transaction callback clears the descriptors to match. Four entries rather than one, and the cap measured rather than derived: - A sequential UPDATE walks rows in row-number order and would be served by one entry, but an index delivers TIDs in index order, and on a column uncorrelated with row order consecutive fetches land in different groups. Measured below. - The cap uses MemoryContextMemAllocated on the entry, because the stored form is encoded and compressed and a group of wide text decodes to many times its size on disk. A group over the cap is used for the fetch that filled it and then dropped, so an outsized group costs what it always did. Measured on PG18, one row group, whole-table UPDATE: rows before after 5,000 4,322 ms 407 ms 10.6x 10,000 18,416 ms 1,564 ms 11.8x 20,000 57,393 ms 5,228 ms 11.0x Index-driven UPDATE of 2,000 rows across ten row groups: row-ordered index 1,257 ms -> 157 ms 8.0x scattered index 1,247 ms -> 815 ms 1.5x This is a constant-factor fix, not an asymptotic one. Doubling the rows in one group still multiplies the time by about 3.3 to 3.8, because a cache hit skips the read and the decode but still walks to the row's position, and that walk is linear in the offset. Option C removes it; the ratio the plan asks for needs both. Co-Authored-By: Claude Opus 5 (1M context) --- src/columnar.h | 1 + src/columnar_reader.c | 252 ++++++++++++++++++++++++++++++++++++----- src/columnar_tableam.c | 1 + 3 files changed, 227 insertions(+), 27 deletions(-) diff --git a/src/columnar.h b/src/columnar.h index f7297a2..471ea61 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -328,6 +328,7 @@ extern void ColumnarVMClearVisible(Relation rel, BlockNumber blk); extern void ColumnarVMClearForRow(Relation rel, uint64 rowNumber); extern bool ColumnarVMIsVisible(Relation rel, BlockNumber blk); extern void ColumnarVMSetVisibleForRelation(Relation rel); +extern void ColumnarDiscardFetchCache(void); /* a contiguous run of all-visible row numbers (gap 28 phase 3) */ typedef struct ColumnarRowRange diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 64fe36b..a9f49d9 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -18,6 +18,7 @@ #include "access/htup_details.h" #include "access/relscan.h" #include "access/tupmacs.h" +#include "access/xact.h" #include "miscadmin.h" #include "port/atomics.h" #include "utils/memutils.h" @@ -1267,6 +1268,155 @@ ColumnarFreeLivenessCache(ColumnarLivenessCache *cache) * to the row's position. Returns false when no stripe covers the row or * the row is marked deleted in the delete vector (spec 7.5). */ + +/* ------------------------------------------------------------------------- + * Statement-scoped decoded row-group cache (issue #143). + * + * ColumnarReadRowByNumber() read and decoded a whole row group to return one + * row, so fetching N rows out of one group cost N times the group: measured at + * 878 ms for 5,000 rows, 4,452 ms for 10,000 and 19,211 ms for 20,000, all in a + * single group. Every index scan, bitmap scan, and index-driven UPDATE or DELETE + * goes through this path. + * + * Correctness comes from the scope rather than from an invalidation protocol. An + * entry is only used within the command that filled it, and a row group's bytes + * are immutable once written, so nothing can rewrite what a cached entry holds + * while that entry is live: a rewrite or compaction needs a lock this statement + * already excludes, and a reclaim that reuses a file offset can only happen in a + * later command, which the command id check rejects. Keying on the storage id as + * well means new storage is never served an old decode. + * + * Visibility is unaffected because it is not cached. The delete vector, the + * buffered delete marks and the validity bitmap are consulted per fetch; only + * decoded column values are held here. + * + * Entries live in contexts under TopTransactionContext, so an abort or commit + * frees them without a hook; ColumnarDiscardFetchCache() clears the descriptors + * to match. + * ------------------------------------------------------------------------- */ + +/* + * Four entries rather than one. A sequential UPDATE walks rows in row-number + * order and stays inside one group, which one entry would serve, but an index + * delivers TIDs in index order, and on a column uncorrelated with row order + * consecutive fetches land in different groups. One entry hits about 1/G of the + * time there; a handful covers it and costs nothing measurable. + */ +#define COLUMNAR_FETCH_CACHE_ENTRIES 4 + +/* + * Cap on the decoded size held at once, measured with MemoryContextMemAllocated + * rather than derived from the stored byte length: the stored form is encoded and + * compressed, so a group 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. + */ +#define COLUMNAR_FETCH_CACHE_MAX_BYTES (32 * 1024 * 1024) + +typedef struct ColumnarFetchGroup +{ + MemoryContext cx; /* holds every pointer below; NULL when free */ + uint64 storageId; + uint64 groupNumber; + CommandId cid; /* the command that filled this entry */ + uint64 firstRowNumber; + uint64 rowCount; + uint64 fileOffset; + int natts; + char *groupBuffer; /* the group's raw bytes */ + NativeColumnChunkMetadata **ccForCol; /* [natts] */ + char **rawBuf; /* [natts]; NULL until that column is decoded */ + uint64 lastUsed; +} ColumnarFetchGroup; + +static ColumnarFetchGroup columnarFetchCache[COLUMNAR_FETCH_CACHE_ENTRIES]; +static uint64 columnarFetchClock = 0; + +/* drop one entry and everything it holds */ +static void +columnar_fetch_entry_reset(ColumnarFetchGroup *e) +{ + if (e->cx != NULL) + MemoryContextDelete(e->cx); + memset(e, 0, sizeof(*e)); +} + +/* + * ColumnarDiscardFetchCache + * Forget every cached group. The contexts hang off TopTransactionContext + * and are already gone by the time this runs at transaction end, so this + * only clears the descriptors that pointed at them. + */ +void +ColumnarDiscardFetchCache(void) +{ + memset(columnarFetchCache, 0, sizeof(columnarFetchCache)); + columnarFetchClock = 0; +} + +/* + * Find the entry for this group in this command, or prepare an empty one. + * Returns NULL when nothing should be cached, in which case the caller decodes + * into its own scratch context exactly as before. + */ +static ColumnarFetchGroup * +columnar_fetch_group_slot(uint64 storageId, uint64 groupNumber, bool *hit) +{ + CommandId cid = GetCurrentCommandId(false); + ColumnarFetchGroup *victim = NULL; + int i; + + *hit = false; + + for (i = 0; i < COLUMNAR_FETCH_CACHE_ENTRIES; i++) + { + ColumnarFetchGroup *e = &columnarFetchCache[i]; + + if (e->cx == NULL) + { + if (victim == NULL) + victim = e; + continue; + } + /* an entry from an earlier command can never be used again */ + if (e->cid != cid) + { + columnar_fetch_entry_reset(e); + if (victim == NULL) + victim = e; + continue; + } + if (e->storageId == storageId && e->groupNumber == groupNumber) + { + e->lastUsed = ++columnarFetchClock; + *hit = true; + return e; + } + } + + if (victim == NULL) + { + /* every slot is live in this command: take the least recently used */ + uint64 oldest = UINT64_MAX; + + for (i = 0; i < COLUMNAR_FETCH_CACHE_ENTRIES; i++) + if (columnarFetchCache[i].lastUsed < oldest) + { + oldest = columnarFetchCache[i].lastUsed; + victim = &columnarFetchCache[i]; + } + columnar_fetch_entry_reset(victim); + } + + victim->cx = AllocSetContextCreate(TopTransactionContext, + "columnar fetch group", + ALLOCSET_DEFAULT_SIZES); + victim->storageId = storageId; + victim->groupNumber = groupNumber; + victim->cid = cid; + victim->lastUsed = ++columnarFetchClock; + return victim; +} + bool ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, Datum *values, bool *nulls) @@ -1280,9 +1430,8 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, Snapshot metaSnapshot; List *rgList; NativeRowGroupMetadata *rg = NULL; - List *nchunks; - NativeColumnChunkMetadata **ccForCol; - char *groupBuffer; + ColumnarFetchGroup *entry; + bool hit; int validityBytes; uint64 rowInGrp; ListCell *nlc; @@ -1299,6 +1448,9 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, * and reconstruct each column's value at its position. Index and bitmap scans * and unique enforcement call this. A deleted row (in the group's delete vector or * a not-yet-flushed buffered delete) is not visible. + * + * The row-group list is read per fetch and deliberately not cached: a group + * flushed earlier in this same statement has to become visible here. */ rgList = ColumnarReadRowGroupList(storageId, metaSnapshot); foreach(nlc, rgList) @@ -1343,27 +1495,55 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, } } - groupBuffer = palloc(rg->byteLength > 0 ? rg->byteLength : 1); - if (rg->byteLength > 0) - ColumnarReadLogicalData(rel, rg->fileOffset, groupBuffer, - rg->byteLength); - nchunks = ColumnarReadColumnChunkList(storageId, rg->groupNumber, - metaSnapshot); - validityBytes = (int) ((rg->rowCount + 7) / 8); - - ccForCol = palloc0(sizeof(NativeColumnChunkMetadata *) * natts); - foreach(nlc, nchunks) + /* + * The group's bytes and its decoded columns are the expensive part and the + * part that repeats across fetches of the same group, so they come from the + * statement-scoped cache above. A miss fills the entry; a hit skips the read + * and the decode entirely. + */ + entry = columnar_fetch_group_slot(storageId, rg->groupNumber, &hit); + if (!hit) { - NativeColumnChunkMetadata *cc = (NativeColumnChunkMetadata *) lfirst(nlc); + MemoryContext entryOld = MemoryContextSwitchTo(entry->cx); + List *nchunks; + + entry->firstRowNumber = rg->firstRowNumber; + entry->rowCount = rg->rowCount; + entry->fileOffset = rg->fileOffset; + entry->natts = natts; + entry->groupBuffer = palloc(rg->byteLength > 0 ? rg->byteLength : 1); + entry->ccForCol = palloc0(sizeof(NativeColumnChunkMetadata *) * natts); + entry->rawBuf = palloc0(sizeof(char *) * natts); + MemoryContextSwitchTo(tmp); + + if (rg->byteLength > 0) + ColumnarReadLogicalData(rel, rg->fileOffset, entry->groupBuffer, + rg->byteLength); + + nchunks = ColumnarReadColumnChunkList(storageId, rg->groupNumber, + metaSnapshot); + foreach(nlc, nchunks) + { + NativeColumnChunkMetadata *cc = (NativeColumnChunkMetadata *) lfirst(nlc); - if (cc->columnIndex >= 0 && cc->columnIndex < natts) - ccForCol[cc->columnIndex] = cc; + if (cc->columnIndex >= 0 && cc->columnIndex < natts) + { + MemoryContextSwitchTo(entry->cx); + entry->ccForCol[cc->columnIndex] = + (NativeColumnChunkMetadata *) palloc(sizeof(*cc)); + memcpy(entry->ccForCol[cc->columnIndex], cc, sizeof(*cc)); + MemoryContextSwitchTo(tmp); + } + } + MemoryContextSwitchTo(entryOld); } + validityBytes = (int) ((entry->rowCount + 7) / 8); + for (c = 0; c < natts; c++) { Form_pg_attribute att = TupleDescAttr(tupdesc, c); - NativeColumnChunkMetadata *cc = ccForCol[c]; + NativeColumnChunkMetadata *cc = entry->ccForCol[c]; char *base; char *vbits; char *rawBuf; @@ -1377,7 +1557,7 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, continue; } - base = groupBuffer + (cc->pageOffset - rg->fileOffset); + base = entry->groupBuffer + (cc->pageOffset - entry->fileOffset); vbits = base; if (((vbits[rowInGrp >> 3] >> (rowInGrp & 7)) & 1) == 0) { @@ -1391,15 +1571,24 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, if ((vbits[i >> 3] >> (i & 7)) & 1) present++; - if (cc->encodingDescriptorLen == 1 && - (uint8) cc->encodingDescriptor[0] == COLUMNAR_NATIVE_ENCDESC_BASELINE) - rawBuf = base + validityBytes; - else - rawBuf = columnar_native_decode_chunk(tmp, att, base + validityBytes, - (uint32) (cc->pageLength - validityBytes), - cc->encodingDescriptor, - cc->encodingDescriptorLen, - cc->blockCodec, NULL, NULL); + if (entry->rawBuf[c] == NULL) + { + MemoryContext decOld = MemoryContextSwitchTo(entry->cx); + + if (cc->encodingDescriptorLen == 1 && + (uint8) cc->encodingDescriptor[0] == COLUMNAR_NATIVE_ENCDESC_BASELINE) + entry->rawBuf[c] = base + validityBytes; + else + entry->rawBuf[c] = + columnar_native_decode_chunk(entry->cx, att, + base + validityBytes, + (uint32) (cc->pageLength - validityBytes), + cc->encodingDescriptor, + cc->encodingDescriptorLen, + cc->blockCodec, NULL, NULL); + MemoryContextSwitchTo(decOld); + } + rawBuf = entry->rawBuf[c]; cursor = rawBuf; for (i = 0; i < present; i++) @@ -1408,6 +1597,15 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, nulls[c] = false; } + /* + * 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. + */ + if (MemoryContextMemAllocated(entry->cx, true) > COLUMNAR_FETCH_CACHE_MAX_BYTES) + columnar_fetch_entry_reset(entry); + MemoryContextSwitchTo(oldContext); MemoryContextDelete(tmp); return true; diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 79b9c1b..37fe392 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -926,6 +926,7 @@ columnar_xact_callback(XactEvent event, void *arg) case XACT_EVENT_PARALLEL_ABORT: ColumnarDiscardAllPendingWrites(); ColumnarDiscardAllDeleteVectors(); + ColumnarDiscardFetchCache(); break; default: break; From 851cec501901d9f8f764f0eac83ed6ee8e19b543 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sat, 25 Jul 2026 18:32:55 -0600 Subject: [PATCH 2/5] Address review: check the invariant, scope retention to the statement, add a test From jdatcmd's review of #148: - The geometry the entry was filled with is now compared against the row group read from the catalog on every hit, using the two fields that were stored and never read. The invariant is almost certainly true, but validityBytes comes from the cached rowCount and base from the cached fileOffset, so a mismatch would read at wrong offsets and return wrong values rather than fail. Four comparisons turn that into a re-decode. - Say at the cap why dropping the entry there is safe: the returned value is decoded into the caller's context and the walk's throwaways into tmp, so nothing handed back points into the context being deleted. - Release the cache in columnar_executor_end rather than only at transaction end, so retention matches the name. Without it a statement that filled all four slots pinned them for the rest of the transaction, and a session idle in transaction held them indefinitely. On the test, the mutation asked for was run first and the result shaped what got written. Removing the command-id rejection fails nothing: native_index, native_dml, unique_conc, differential, native_compact, native_recluster, concurrent_diff and native_rewrite all pass with it gone, because four other things independently prevent a stale hit. The storage id is in the key, so a rewrite misses. Compaction retires group numbers rather than reusing them, so an in-place compact misses too (measured: group numbers and offsets are unchanged across pgcolumnar.compact). The new geometry check rejects a mismatched entry. And the executor-end release drops the cache between statements. So test/native_fetch_cache.sh asserts two things with two kinds of evidence: a ratio for the behaviour, which fails without the cache (11,206 ms against 1,001 ms for the same 2,000 fetches, against 131 and 129 with it), and source assertions for the four scoping guards, in the style of wal_envelope.sh and decode_interrupts.sh, since a behavioural test for any one guard would have to defeat the other three. Co-Authored-By: Claude Opus 5 (1M context) --- docs/testing.md | 1 + src/columnar_reader.c | 27 +++++++++++ src/columnar_tableam.c | 8 +++ test/native_fetch_cache.sh | 99 ++++++++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 2 +- 5 files changed, 136 insertions(+), 1 deletion(-) create mode 100755 test/native_fetch_cache.sh diff --git a/docs/testing.md b/docs/testing.md index 0863a9d..ad7d846 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -46,6 +46,7 @@ test/native_parquet_partition.sh /path/to/pg_config # Hive partition columns an test/native_cancel.sh /path/to/pg_config # scan cancellation during a group load test/wal_envelope.sh /path/to/pg_config # WAL discipline: core mechanisms only test/decode_interrupts.sh /path/to/pg_config # decode path stays interruptible +test/native_fetch_cache.sh /path/to/pg_config # fetch-by-row-number group cache test/native_writer.sh /path/to/pg_config # native format catalog output test/native_roundtrip.sh /path/to/pg_config # native write then read round-trip test/native_encoding.sh /path/to/pg_config # native per-vector encoding cascade diff --git a/src/columnar_reader.c b/src/columnar_reader.c index a9f49d9..413ef3d 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -1502,6 +1502,28 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, * and the decode entirely. */ entry = columnar_fetch_group_slot(storageId, rg->groupNumber, &hit); + + /* + * The geometry the entry was filled with has to match the group just read + * out of the catalog. The invariant that it always does is argued above and + * is almost certainly true, but it is load-bearing rather than decorative: + * validityBytes comes from the cached rowCount and base from the cached + * fileOffset, so a group number that ever came back with different geometry + * inside one command would be read at wrong offsets and return wrong values + * rather than fail. Checking costs four comparisons and turns that into a + * re-decode. + */ + if (hit && + (entry->firstRowNumber != rg->firstRowNumber || + entry->rowCount != rg->rowCount || + entry->fileOffset != rg->fileOffset || + entry->natts != natts)) + { + columnar_fetch_entry_reset(entry); + entry = columnar_fetch_group_slot(storageId, rg->groupNumber, &hit); + Assert(!hit); + } + if (!hit) { MemoryContext entryOld = MemoryContextSwitchTo(entry->cx); @@ -1602,6 +1624,11 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, * 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, and the throwaway values the walk produces + * go into tmp. Only the decoded stream itself lives in entry->cx. */ if (MemoryContextMemAllocated(entry->cx, true) > COLUMNAR_FETCH_CACHE_MAX_BYTES) columnar_fetch_entry_reset(entry); diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 37fe392..f8b6689 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -978,6 +978,14 @@ columnar_executor_end(QueryDesc *queryDesc) ColumnarFlushAllPendingWrites(); ColumnarFlushAllDeleteVectors(); + + /* + * The fetch cache is scoped to a statement, so release it here rather than + * waiting for transaction end. Without this a statement that filled every + * slot pins them for the rest of the transaction, and a session sitting idle + * in transaction after one UPDATE holds them indefinitely. + */ + ColumnarDiscardFetchCache(); } /* ------------------------------------------------------------------------- diff --git a/test/native_fetch_cache.sh b/test/native_fetch_cache.sh new file mode 100755 index 0000000..4455c27 --- /dev/null +++ b/test/native_fetch_cache.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# +# pgColumnar fetch-by-row-number cache (issue #143). +# +# ColumnarReadRowByNumber() used to read and decode a whole row group per row +# returned, so fetching N rows out of one group cost N times the group. A +# statement-scoped cache of the decoded group removes the repeat. +# +# Two things are asserted, because they need different kinds of evidence. +# +# 1. The cache is used. Timed as a ratio rather than a threshold: the same number +# of index-driven fetches is run against the same rows laid out as one big row +# group and as many small ones. Without the cache the per-fetch cost is +# proportional to the group size, so the single-group case is many times +# slower; with it, both are dominated by the fetches themselves and the ratio +# collapses. A ratio is portable where a millisecond count is not. +# +# 2. The scoping that makes the cache correct is present. This part is a source +# assertion, in the style of wal_envelope.sh and decode_interrupts.sh, and the +# reason is worth recording: removing the command-id rejection on its own does +# not fail any suite, because four other things independently prevent a stale +# hit (the storage id is in the key, compaction retires group numbers rather +# than rewriting them under the same number, the geometry check rejects a +# mismatched entry, and the executor-end hook releases the cache between +# statements). A behavioural test for that one guard would have to defeat the +# other four, which is not a shape the suite should carry. Asserting the guards +# exist is the honest cover for them. +# +# Usage: test/native_fetch_cache.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +ROWS=${PGC_FETCH_ROWS:-20000} +UPD=$((ROWS / 10)) + +# --- 1. the cache is used ----------------------------------------------------- + +build() { # table, rows-per-group + psql_run "DROP TABLE IF EXISTS $1; + SET pgcolumnar.stripe_row_limit = $2; + SET pgcolumnar.chunk_group_row_limit = $2; + CREATE TABLE $1 (id int, v int, t text) USING pgcolumnar; + INSERT INTO $1 SELECT g, g, 'row' || g FROM generate_series(1, $ROWS) g; + CREATE INDEX ${1}_id ON $1 (id);" +} + +# index-driven so every row goes through the fetch path rather than a scan +upd_ms() { + local start end + start=$(date +%s%N) + psql_run "SET max_parallel_workers_per_gather = 0; + SET enable_seqscan = off; + SET enable_bitmapscan = off; + UPDATE $1 SET v = v + 1 WHERE id <= $UPD;" >/dev/null 2>&1 + end=$(date +%s%N) + echo $(( (end - start) / 1000000 )) +} + +build fc_one "$ROWS" # every row in a single row group +build fc_many $((ROWS / 10)) # the same rows across ten + +one="$(upd_ms fc_one)" +many="$(upd_ms fc_many)" +echo "-- $UPD fetches: one group of $ROWS took ${one} ms; ten groups took ${many} ms" + +# Without the cache the single-group case decodes ten times as much per fetch and +# lands near 10x. With it, both are dominated by the fetches and sit near 1x. +check "fetching from one big group is not far dearer than from ten small ones" \ + "$( [ "$many" -gt 0 ] && [ $(( one / (many > 0 ? many : 1) )) -lt 3 ] && echo yes || + echo "no (one=${one}ms ten=${many}ms)")" \ + "yes" + +# and the rows are still right +check "the updated rows are correct" \ + "$(q "SELECT count(*) FROM fc_one WHERE v = id + 1;")" "$UPD" +check "the untouched rows are unchanged" \ + "$(q "SELECT count(*) FROM fc_one WHERE v = id;")" "$((ROWS - UPD))" + +# --- 2. the scoping guards are present ---------------------------------------- + +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" + +check "the entry key includes the storage id" \ + "$(grep -c 'e->storageId == storageId && e->groupNumber == groupNumber' "$SRC/columnar_reader.c")" "1" + +check "an entry from an earlier command is rejected" \ + "$(grep -c 'e->cid != cid' "$SRC/columnar_reader.c")" "1" + +check "a hit re-checks the group geometry it was filled with" \ + "$(grep -cE 'entry->fileOffset != rg->fileOffset' "$SRC/columnar_reader.c")" "1" + +check "the cache is released at executor end, not only at transaction end" \ + "$(grep -c 'ColumnarDiscardFetchCache' "$SRC/columnar_tableam.c")" "2" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 122bdfd..8cfe1b5 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -27,7 +27,7 @@ SRCDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" SUITES=(harness_selftest smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ differential recovery fuzz hardening concurrent_diff parallel sorted_projection \ arrow_export parquet_export read_stream corruption \ - generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_zonemap native_skip native_agg native_bloom native_vecskip native_index native_dml native_ios native_projection native_cluster native_compact native_recluster native_reclaim native_ownership native_reclaim_cycles native_reclaim_frag native_gap native_truncate native_rewrite native_rewrite_conc native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel wal_envelope decode_interrupts isolation) + generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_zonemap native_skip native_agg native_bloom native_vecskip native_index native_dml native_ios native_projection native_cluster native_compact native_recluster native_reclaim native_ownership native_reclaim_cycles native_reclaim_frag native_gap native_truncate native_rewrite native_rewrite_conc native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel wal_envelope decode_interrupts native_fetch_cache isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=( From 267de4c90b6f4aad2470f4812abeced80fda6a28 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 25 Jul 2026 18:59:44 -0600 Subject: [PATCH 3/5] Copy the encoding descriptor into the cache entry, not just the pointer NativeColumnChunkMetadata.encodingDescriptor points at the bytea the catalog scan produced, which is allocated in the per-call scratch context and freed when that context is deleted at the end of the fetch. The entry copied the struct and kept the pointer, so every later hit on that group read a decoding descriptor out of freed memory. It usually works, which is what makes it dangerous: freed memory normally still holds the old bytes. The projections suite is where it stopped working, failing with "unrecognized native encoding descriptor" from reconstruct_via_projection on both PG18 and PG19. The same read landing on reused memory that happens to parse would return wrong values with no error at all. The descriptor bytes now belong to the entry. Full suite green on PG18 and PG19 afterwards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012uKWWwBDt5TWWS5DR2tzDb --- src/columnar_reader.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 413ef3d..e077d4f 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -1550,10 +1550,33 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, if (cc->columnIndex >= 0 && cc->columnIndex < natts) { + NativeColumnChunkMetadata *copy; + MemoryContextSwitchTo(entry->cx); - entry->ccForCol[cc->columnIndex] = - (NativeColumnChunkMetadata *) palloc(sizeof(*cc)); - memcpy(entry->ccForCol[cc->columnIndex], cc, sizeof(*cc)); + copy = (NativeColumnChunkMetadata *) palloc(sizeof(*cc)); + memcpy(copy, cc, sizeof(*cc)); + + /* + * encodingDescriptor is a pointer into the bytea the catalog + * scan produced, which lives in tmp and dies with it at the end + * of this call. Copying the struct alone leaves every later hit + * reading freed memory, which usually still holds the old bytes + * and so usually works: the projections suite caught it as + * "unrecognized native encoding descriptor", but freed memory + * that happens to decode is the same bug returning wrong values + * in silence. The descriptor comes with the entry. + */ + if (cc->encodingDescriptor != NULL && + cc->encodingDescriptorLen > 0) + { + char *desc = (char *) palloc(cc->encodingDescriptorLen); + + memcpy(desc, cc->encodingDescriptor, + cc->encodingDescriptorLen); + copy->encodingDescriptor = desc; + } + + entry->ccForCol[cc->columnIndex] = copy; MemoryContextSwitchTo(tmp); } } From d5c52a78a498480104bdd1d48cc1704bd554007c Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 25 Jul 2026 19:00:59 -0600 Subject: [PATCH 4/5] test: pin the allocator invariants the cache's safety argument rests on native_fetch_cache.sh reasons that a stale hit is prevented in part because compaction retires group numbers rather than reusing them, and because the storage id is part of the key. Both are properties of the allocator, and neither is asserted anywhere, so a change to either would move the ground under the cache without failing a test. They belong in native_rewrite, since that is where such a change would be made: no group number is reused across a rewrite and subsequent writes, numbers issued afterwards are past every earlier one, and vacuum rewrites into a fresh storage id. Freezing the metapage counter that issues group numbers fails the first two. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012uKWWwBDt5TWWS5DR2tzDb --- test/native_rewrite.sh | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/native_rewrite.sh b/test/native_rewrite.sh index f56619b..ec1d42d 100755 --- a/test/native_rewrite.sh +++ b/test/native_rewrite.sh @@ -92,4 +92,53 @@ SQL check "compact_rewrite holds ShareUpdateExclusiveLock" "${locks%%|*}" "1" check "compact_rewrite holds no AccessExclusiveLock" "${locks##*|}" "0" +# --------------------------------------------------------------------------- +# What makes the statement-scoped fetch cache safe (#143, #148). +# +# ColumnarReadRowByNumber caches a decoded row group keyed by +# (storage_id, group_number). That is only sound because such a pair is never +# re-served with different bytes, which rests on two allocator properties rather +# than on any locking: +# +# group numbers come from the monotonic metapage counter reserved in +# ColumnarReserveRowNumbers (meta->reservedStripeId += 1), never from the +# catalog maximum, so retiring a group does not free its number for reuse; +# +# a rewrite writes into freshly allocated storage rather than over the old. +# +# Both are asserted here rather than in the fetch path, because this is where a +# change would be made. If either stops holding, the cache can hand back a decode +# of bytes that no longer exist and these checks are the warning. +# --------------------------------------------------------------------------- + +before_max="$(q "SELECT coalesce(max(group_number), -1) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('n');")" +psql_run "SELECT pgcolumnar.compact_rewrite('n', 0.1);" >/dev/null +# Fresh ids: n carries a unique index, so re-inserting the original range would +# fail and leave no new groups to look at. +EXTRA="SELECT g + 1000000, g % 1000, 'p' || (g % 100) FROM generate_series(1, 2000) g" +psql_run "INSERT INTO n $EXTRA;" +psql_run "INSERT INTO h $EXTRA;" +after_min_new="$(q "SELECT coalesce(min(group_number), -1) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('n') + AND group_number > $before_max;")" + +check "a rewrite plus later writes reuse no group number" \ + "$(q "SELECT count(*) = count(DISTINCT group_number) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('n');")" "t" +check "group numbers issued after a rewrite are past every earlier one" \ + "$([ "$after_min_new" -gt "$before_max" ] && echo yes || echo no)" "yes" + +# A full rewrite moves the table to new storage, so a cached decode keyed by the +# old storage id can never match afterwards. +sid_before="$(q "SELECT pgcolumnar.get_storage_id('n');")" +psql_run "SELECT pgcolumnar.vacuum('n');" >/dev/null +sid_after="$(q "SELECT pgcolumnar.get_storage_id('n');")" +check "vacuum rewrites into a fresh storage id" \ + "$([ "$sid_after" != "$sid_before" ] && echo yes || echo no)" "yes" + +# And the rows survive all of it, which is what the invariants are for. +check "every live row still reads back after rewrite and vacuum" \ + "$(q 'SELECT count(*) FROM n;')" "$(q 'SELECT count(*) FROM h;')" + pgc_summary From eba87a433d8a1b08a3728eebe3fb5dcf4d79beb5 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sat, 25 Jul 2026 19:09:55 -0600 Subject: [PATCH 5/5] Correct the cache's safety argument: it is not a lock argument jdatcmd is right that the header claimed a rewrite or compaction 'needs a lock this statement already excludes'. pgcolumnar.compact_rewrite runs under ShareUpdateExclusiveLock and is documented as concurrent with readers and writers, so a compaction can run while an entry is live. The cache is safe for the reasons the test now lists, not that one. Replaces the claim with the four things that actually prevent a stale hit, and says explicitly that it is not a lock argument so the next person does not lean on the one that does not hold. Co-Authored-By: Claude Opus 5 (1M context) --- src/columnar_reader.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/columnar_reader.c b/src/columnar_reader.c index e077d4f..500c230 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -1278,13 +1278,25 @@ ColumnarFreeLivenessCache(ColumnarLivenessCache *cache) * single group. Every index scan, bitmap scan, and index-driven UPDATE or DELETE * goes through this path. * - * Correctness comes from the scope rather than from an invalidation protocol. An - * entry is only used within the command that filled it, and a row group's bytes - * are immutable once written, so nothing can rewrite what a cached entry holds - * while that entry is live: a rewrite or compaction needs a lock this statement - * already excludes, and a reclaim that reuses a file offset can only happen in a - * later command, which the command id check rejects. Keying on the storage id as - * well means new storage is never served an old decode. + * Correctness comes from the scope rather than from an invalidation protocol. A + * row group's bytes are immutable once written, and four independent things keep + * a stale entry from being used: + * + * - the storage id is part of the key, so anything that allocates new storage + * (pgcolumnar.vacuum, and any rewrite that goes through a new relfilenode) + * misses rather than matching; + * - a rewrite retires group numbers rather than reusing them, so a compacted + * group never reappears under its old number with different content; + * - the geometry an entry was filled with is re-checked against the catalog on + * every hit, so a group number that did come back with a different shape is + * treated as a miss; + * - and an entry is only used within the command that filled it. + * + * Note it is NOT a lock argument. pgcolumnar.compact_rewrite runs under + * ShareUpdateExclusiveLock and does not conflict with a reader, so a concurrent + * compaction is possible while this cache is live; the reasons above are what + * make that safe, and test/native_rewrite.sh pins the two of them that are + * properties of the allocator rather than of this file. * * Visibility is unaffected because it is not cached. The delete vector, the * buffered delete marks and the validity bitmap are consulted per fetch; only