From 0ad4ed4167882e430b56d3518922a797cf579060 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 31 Jul 2026 14:17:40 -0600 Subject: [PATCH] Remove the dead column cache (#303) The cache had not run since 22 July. ColumnarGetDecompressedStream, its only entry point, had no callers: Phase H2 removed the old on-disk format and took the call site with it, and nothing since put one back. What executed was ColumnarCacheInit from _PG_init, which is the whole of the 6.7% line coverage that #282 asked about. Two settings and four passages of documentation described a feature that did nothing. A user could set pgcolumnar.enable_column_cache, read the administration guide, and reasonably believe they had turned something on. Removed rather than re-wired, and the reason is that this is not a wire to reconnect. The code was written against the format H2 deleted; the current native reader decompresses on a different path, so restoring the behaviour means writing a new integration and making the performance case again. Its value now is as a design sketch, and the git history keeps that. #289's vectorized decompression work would shape any future caching layer more than this code would. The three assertions in phase6.sh go with it, and they are worth naming. They compared query results with the cache on against the cache off and asserted the two were equal, and one claimed in its comment to exercise LRU eviction. All three passed against a cache that did nothing, because a correctness-only comparison between two identical paths cannot fail. columnar_lru_evict has never run. That is the same shape as an empty REGRESS reporting success. A postgresql.conf that sets either parameter must drop the line, which the CHANGELOG records under Removed. Pre-release, no compatibility guarantee, and neither setting did anything. Five-major matrix: ALL VERSIONS PASSED. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 10 ++ Makefile | 1 - docs/ARCHITECTURE.md | 13 +-- docs/administration.md | 8 +- docs/configuration.md | 2 - docs/testing.md | 2 +- src/columnar.h | 16 --- src/columnar_cache.c | 240 ----------------------------------------- src/columnar_tableam.c | 20 ---- test/phase6.sh | 31 ++---- 10 files changed, 23 insertions(+), 320 deletions(-) delete mode 100644 src/columnar_cache.c diff --git a/CHANGELOG.md b/CHANGELOG.md index de1b9b7..ac78634 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,6 +130,16 @@ unreleased. For the forward-looking plan see - Lost delete marks under concurrent same-chunk-group deletes. - Relation-reference leak in parallel `CREATE INDEX`. +### Removed + +- The decompressed-chunk cache, and the `pgcolumnar.enable_column_cache` and + `pgcolumnar.column_cache_size` settings with it. Its only entry point had lost + its caller when the earlier on-disk format was removed, so the cache had done + nothing since. Two settings and four passages of documentation described a + feature that did not run. A `postgresql.conf` that sets either parameter must + drop the line. The implementation is in the git history if the performance case + is made again against the current reader. + ### Changed - FSST string encoding is now kept only when it reduces the compressed chunk by diff --git a/Makefile b/Makefile index d82bfe8..9e1ffa0 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,6 @@ OBJS = \ src/columnar_reader.o \ src/columnar_delete_vector.o \ src/columnar_customscan.o \ - src/columnar_cache.o \ src/columnar_vector.o \ src/columnar_vacuum.o \ src/columnar_unique.o \ diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b1cdd00..224e83a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -75,7 +75,7 @@ storage, bulk and single insert, sequential scan open/next/close, delete and update (through the row mask), fetch a row by item pointer, size estimation, and truncate. It also holds `_PG_init`, which registers every `pgcolumnar.*` GUC (the compression codec and level, the row-group and vector row limits, and the qual -pushdown, custom scan, vectorized aggregate, and column cache toggles), the +pushdown, custom scan and vectorized aggregate toggles), the pre-commit hook that flushes pending writes, and the object-access hook that removes a table's metadata rows when the table is dropped. @@ -208,15 +208,6 @@ answered from the zone-map metadata, falling back to a scan-and-fold when the group has deletes. It is chosen only when every aggregate, column type, and clause is supported; anything else falls back to the scalar plan. -### columnar_cache.c -The optional decompressed-chunk cache, off by default behind -`pgcolumnar.enable_column_cache` and bounded by `pgcolumnar.column_cache_size` -megabytes. It is a backend-local, LRU-bounded cache of decompressed value -streams keyed by storage id and absolute logical offset. It returns a fresh copy -to the caller so eviction is always safe, and it is flushed on any relcache -invalidation so a truncate offset reuse or a vacuum storage swap can never serve -a stale buffer. It only avoids repeated decompression; it never changes results. - ### columnar_vacuum.c Compaction, statistics, and storage-id lookup. `pgcolumnar.vacuum` materializes a relation's live rows (the reader skips row-mask-deleted rows), swaps the @@ -373,7 +364,7 @@ Scan: 2. The reader (`columnar_reader`) goes through the row groups. It uses the zone maps in `columnar_metadata` to skip groups and vectors. 3. The reader decodes the projected chunks through `columnar_encoding` and - `columnar_compression`, and can use `columnar_cache`. + `columnar_compression`. 4. The reader applies the delete vector (`columnar_delete_vector`) and returns the rows one at a time. 5. The executor applies the full qual again, as a filter. diff --git a/docs/administration.md b/docs/administration.md index 2485760..99de2f8 100644 --- a/docs/administration.md +++ b/docs/administration.md @@ -205,11 +205,9 @@ specific reason to change it. ## Column cache -`pgcolumnar.enable_column_cache` keeps chunk groups after decompression. Other -reads can then use them again. `pgcolumnar.column_cache_size` sets the size, and -the default is 200 MB. The cache is off by default. Enable it if you scan the -same recent data more than one time. Set the size to the size of the working -set. +There is no cache of decompressed chunk groups. A cache existed in an earlier +build, but its only entry point lost its caller and the code did nothing. It was +removed in #303 rather than left as a setting that changes nothing. ## Backup and restore diff --git a/docs/configuration.md b/docs/configuration.md index a5f83ed..ace376c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -69,8 +69,6 @@ disk. It never changes the values that a table returns. | Setting | Type | Default | Description | | --- | --- | --- | --- | -| `pgcolumnar.enable_column_cache` | boolean | `off` | Cache decompressed chunk groups so they can be reused across reads. | -| `pgcolumnar.column_cache_size` | integer (MB) | `200` | Size of the decompressed-chunk cache. Applies when the column cache is enabled. Range 1 to INT_MAX. | ### Maintenance and disk reclaim diff --git a/docs/testing.md b/docs/testing.md index 32cc752..03986c3 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -10,7 +10,7 @@ test/phase2.sh /path/to/pg_config # compression, projection, min/max skip, fi test/phase3.sh /path/to/pg_config # delete, update, MVCC, savepoints, temp tables test/phase4.sh /path/to/pg_config # btree/hash indexes, constraints, conversion test/phase5.sh /path/to/pg_config # custom scan, pushdown, options, vacuum -test/phase6.sh /path/to/pg_config # aggregate correctness and the column cache +test/phase6.sh /path/to/pg_config # aggregate correctness test/audit.sh /path/to/pg_config # regression tests for audited defects test/concurrency.sh /path/to/pg_config # concurrent same-chunk-group deletes test/unique_conc.sh /path/to/pg_config # concurrent same-unique-key inserts diff --git a/src/columnar.h b/src/columnar.h index 106a832..3b9e4e3 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -173,11 +173,9 @@ extern bool columnar_enable_bloom_filter; /* bloom equality skipping (I7) */ /* Phase 6 GUCs (spec 8.3) */ extern bool columnar_enable_vectorization; /* vectorized aggregate path */ -extern bool columnar_enable_column_cache; /* decompressed-chunk cache */ extern bool columnar_enable_read_stream; /* stream/prefetch block reads (PG17+) */ extern bool columnar_enable_index_only_scan; /* allow index-only scans (gap 28) */ extern bool columnar_enable_projection_scan; /* scan a covering projection (gap 26) */ -extern int columnar_column_cache_size; /* cache budget in megabytes */ /* issue #5: concurrent unique-key insert serialization */ extern bool columnar_enable_unique_lock; /* serialize same-key inserters */ @@ -741,20 +739,6 @@ extern char *ColumnarDecompressValueStream(const char *comp, uint32 compLen, int compressionType, uint32 rawLen, MemoryContext targetContext); -/* ------------------------------------------------------------------------- - * decompressed-chunk cache (columnar_cache.c, spec 8.3, 9) - * - * An optional, backend-local cache of decompressed value streams, keyed by the - * relation's storage id and the stream's absolute logical offset (both stable - * and never reused within a storage id, except across a truncate, which fires a - * relcache invalidation that flushes the whole cache). Off by default; when on - * it only avoids repeated decompression and never changes results. - * ------------------------------------------------------------------------- */ -extern void ColumnarCacheInit(void); -extern char *ColumnarGetDecompressedStream(uint64 storageId, uint64 absOffset, - const char *comp, uint32 compLen, - int compressionType, uint32 rawLen, - MemoryContext targetContext); /* ------------------------------------------------------------------------- * concurrent unique-key insert serialization (columnar_unique.c, issue #5) diff --git a/src/columnar_cache.c b/src/columnar_cache.c deleted file mode 100644 index 63f7f81..0000000 --- a/src/columnar_cache.c +++ /dev/null @@ -1,240 +0,0 @@ -/*------------------------------------------------------------------------- - * - * columnar_cache.c - * Optional decompressed-chunk cache for pgColumnar (spec 8.3, 9). - * - * When columnar.enable_column_cache is on, the reader keeps decompressed value - * streams in a backend-local cache so that repeated reads of the same chunk - * group reuse the decompressed bytes instead of decompressing again. The cache - * is a pure optimization: with it on or off, every query returns exactly the - * same rows. It is off by default and bounded by columnar.column_cache_size - * megabytes with least-recently-used eviction. - * - * Safety of the key. A cache entry is keyed by (storageId, absOffset), where - * absOffset is the value stream's absolute logical offset in the relation. A - * stripe's data is written append-only and never rewritten, so within one - * storage id an absolute offset holds exactly one immutable value stream. Two - * events can make a key stale: a truncate resets the metapage so offsets are - * reused, and vacuum swaps to a new storage id. Both fire a relcache - * invalidation, and this module flushes the entire cache on any relcache - * invalidation, so a stale entry is never read. The returned buffer is always a - * fresh copy in the caller's context, so eviction can free cache memory without - * touching a buffer a scan is still using. - * - * Independent MIT implementation built from - * design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md and the public PostgreSQL API only. - * - *------------------------------------------------------------------------- - */ -#include "columnar.h" - -#include "utils/hsearch.h" -#include "utils/inval.h" -#include "utils/memutils.h" - -/* GUCs (spec 8.3); registered in columnar_tableam.c _PG_init */ -bool columnar_enable_column_cache = false; -int columnar_column_cache_size = 200; /* megabytes */ - -typedef struct ColumnarCacheKey -{ - uint64 storageId; - uint64 absOffset; /* stripe file_offset + value_stream_offset */ -} ColumnarCacheKey; - -typedef struct ColumnarCacheEntry -{ - ColumnarCacheKey key; /* must be first: dynahash hashes on it */ - uint32 rawLen; /* decompressed length */ - int compressionType; - char *buffer; /* rawLen decompressed bytes, in cacheContext */ - struct ColumnarCacheEntry *lruPrev; - struct ColumnarCacheEntry *lruNext; -} ColumnarCacheEntry; - -static HTAB *cacheHash = NULL; -static MemoryContext cacheContext = NULL; -static Size cacheBytes = 0; -static ColumnarCacheEntry *lruHead = NULL; /* most recently used */ -static ColumnarCacheEntry *lruTail = NULL; /* least recently used */ - -static void -columnar_cache_flush_all(Datum arg, Oid relid) -{ - /* - * Drop everything on any relcache invalidation. This is heavy-handed but - * always correct, and the events that matter (truncate reusing offsets, - * vacuum swapping storage) are exactly relcache invalidations. Plain reads - * do not invalidate the relcache, so a warm cache stays warm across repeated - * queries that do no DDL. - */ - if (cacheHash == NULL) - return; - - hash_destroy(cacheHash); - cacheHash = NULL; - MemoryContextReset(cacheContext); - cacheBytes = 0; - lruHead = NULL; - lruTail = NULL; -} - -void -ColumnarCacheInit(void) -{ - /* - * Create the long-lived context now; the hash table is created lazily on - * first use (and recreated after a flush). Register the invalidation - * callback once at load time. - */ - cacheContext = AllocSetContextCreate(TopMemoryContext, - "columnar decompressed cache", - ALLOCSET_DEFAULT_SIZES); - CacheRegisterRelcacheCallback(columnar_cache_flush_all, (Datum) 0); -} - -static void -columnar_cache_create_hash(void) -{ - HASHCTL info; - - MemSet(&info, 0, sizeof(info)); - info.keysize = sizeof(ColumnarCacheKey); - info.entrysize = sizeof(ColumnarCacheEntry); - info.hcxt = cacheContext; - - cacheHash = hash_create("columnar decompressed chunk cache", 128, &info, - HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); -} - -/* unlink an entry from the LRU list */ -static void -columnar_lru_unlink(ColumnarCacheEntry *e) -{ - if (e->lruPrev != NULL) - e->lruPrev->lruNext = e->lruNext; - else - lruHead = e->lruNext; - - if (e->lruNext != NULL) - e->lruNext->lruPrev = e->lruPrev; - else - lruTail = e->lruPrev; - - e->lruPrev = NULL; - e->lruNext = NULL; -} - -/* push an entry to the most-recently-used end */ -static void -columnar_lru_push_front(ColumnarCacheEntry *e) -{ - e->lruPrev = NULL; - e->lruNext = lruHead; - if (lruHead != NULL) - lruHead->lruPrev = e; - lruHead = e; - if (lruTail == NULL) - lruTail = e; -} - -static void -columnar_lru_touch(ColumnarCacheEntry *e) -{ - if (lruHead == e) - return; - columnar_lru_unlink(e); - columnar_lru_push_front(e); -} - -/* evict least-recently-used entries until within budget, never evicting keep */ -static void -columnar_cache_evict(Size budget, ColumnarCacheEntry *keep) -{ - while (cacheBytes > budget && lruTail != NULL && lruTail != keep) - { - ColumnarCacheEntry *victim = lruTail; - bool found; - - columnar_lru_unlink(victim); - cacheBytes -= victim->rawLen; - pfree(victim->buffer); - hash_search(cacheHash, &victim->key, HASH_REMOVE, &found); - } -} - -/* - * ColumnarGetDecompressedStream - * Return rawLen decompressed bytes of a value stream in targetContext. With - * the cache off, this simply decompresses. With the cache on, a hit copies - * the cached decompressed bytes (skipping decompression) and a miss - * decompresses and stores a copy for next time. The result is always a - * fresh buffer owned by the caller, so cache eviction is always safe. - */ -char * -ColumnarGetDecompressedStream(uint64 storageId, uint64 absOffset, - const char *comp, uint32 compLen, - int compressionType, uint32 rawLen, - MemoryContext targetContext) -{ - ColumnarCacheKey key; - ColumnarCacheEntry *entry; - bool found; - char *result; - Size budget; - - /* an all-null chunk has an empty value stream (spec 4) */ - if (rawLen == 0) - return NULL; - - if (!columnar_enable_column_cache) - return ColumnarDecompressValueStream(comp, compLen, compressionType, - rawLen, targetContext); - - if (cacheHash == NULL) - columnar_cache_create_hash(); - - key.storageId = storageId; - key.absOffset = absOffset; - - entry = (ColumnarCacheEntry *) hash_search(cacheHash, &key, HASH_FIND, &found); - if (found && entry->rawLen == rawLen && - entry->compressionType == compressionType) - { - result = MemoryContextAlloc(targetContext, rawLen); - memcpy(result, entry->buffer, rawLen); - columnar_lru_touch(entry); - return result; - } - - /* miss (or a defensive key collision): decompress into the caller's buffer */ - result = ColumnarDecompressValueStream(comp, compLen, compressionType, - rawLen, targetContext); - - /* do not cache a single stream larger than the whole budget */ - budget = (Size) columnar_column_cache_size * 1024L * 1024L; - if ((Size) rawLen > budget) - return result; - - /* store a copy in the cache for future reads */ - entry = (ColumnarCacheEntry *) hash_search(cacheHash, &key, HASH_ENTER, &found); - if (found) - { - /* replacing a stale entry for the same key: free its old buffer */ - columnar_lru_unlink(entry); - cacheBytes -= entry->rawLen; - pfree(entry->buffer); - } - entry->rawLen = rawLen; - entry->compressionType = compressionType; - entry->buffer = MemoryContextAlloc(cacheContext, rawLen); - memcpy(entry->buffer, result, rawLen); - entry->lruPrev = NULL; - entry->lruNext = NULL; - columnar_lru_push_front(entry); - cacheBytes += rawLen; - - columnar_cache_evict(budget, entry); - - return result; -} diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index a364f98..034ee5e 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2253,15 +2253,6 @@ _PG_init(void) 0, NULL, NULL, NULL); - DefineCustomBoolVariable("pgcolumnar.enable_column_cache", - "Cache decompressed chunk groups to reuse across reads.", - NULL, - &columnar_enable_column_cache, - false, - PGC_USERSET, - 0, - NULL, NULL, NULL); - DefineCustomBoolVariable("pgcolumnar.reclaim_coalesce", "Split oversized freed ranges on reuse and coalesce " "adjacent freed ranges, so compaction reclaims space " @@ -2314,16 +2305,6 @@ _PG_init(void) 0, NULL, NULL, NULL); - DefineCustomIntVariable("pgcolumnar.column_cache_size", - "Size of the decompressed-chunk cache, in megabytes.", - NULL, - &columnar_column_cache_size, - 200, - 1, INT_MAX, - PGC_USERSET, - GUC_UNIT_MB, - NULL, NULL, NULL); - DefineCustomBoolVariable("pgcolumnar.enable_unique_insert_lock", "Serialize concurrent inserts of the same unique key.", "Takes a transaction-scoped advisory lock per unique " @@ -2388,7 +2369,6 @@ _PG_init(void) ColumnarVectorInit(); /* set up the optional decompressed-chunk cache (spec 8.3) */ - ColumnarCacheInit(); /* register the unique-index cache invalidation callback (issue #5) */ ColumnarUniqueInit(); diff --git a/test/phase6.sh b/test/phase6.sh index d3970af..915d23c 100644 --- a/test/phase6.sh +++ b/test/phase6.sh @@ -110,20 +110,6 @@ eq_on_off() { echo "PASS $name: $on" } -# Run a query with the decompressed-chunk cache on and off; assert equality. -cache_on_off() { - local name="$1" query="$2" - local on off - on="$(run_pg "$PSQL -c \"SET pgcolumnar.enable_column_cache=on; $query\"")" - off="$(run_pg "$PSQL -c \"SET pgcolumnar.enable_column_cache=off; $query\"")" - if [ -z "$on" ] || [ "$on" != "$off" ]; then - echo "FAIL $name: cache-on [$on] != cache-off [$off]" - fail=1 - return - fi - echo "PASS $name: $on" -} - q "CREATE EXTENSION pgcolumnar;" >/dev/null # --------------------------------------------------------------------------- @@ -207,16 +193,13 @@ eq_on_off "min empty is null" "SELECT COALESCE(min(a)::text,'NULL') FROM nt # the answer: cache on and cache off produce identical results, for aggregates # and for a filtered row scan. # --------------------------------------------------------------------------- -echo "-- decompressed-chunk cache on vs off is identical" -cache_on_off "cache agg suite" \ - "SELECT count(*), sum(id), avg(id), min(id), max(id) FROM t WHERE id > 500;" -cache_on_off "cache filtered scan" \ - "SELECT md5(string_agg(id||'|'||label, ',' ORDER BY id)) FROM t WHERE id BETWEEN 20000 AND 21000;" -cache_on_off "cache with nulls" \ - "SELECT count(a), sum(a), min(txt) FROM nt WHERE a > 50;" -# a small cache budget still returns correct results (exercises LRU eviction) -small_on="$(q "SET pgcolumnar.enable_column_cache=on; SET pgcolumnar.column_cache_size=1; SELECT sum(id) FROM t;")" -check "tiny cache still correct" "$small_on" "1250025000" +# The decompressed-chunk cache assertions that stood here are gone with the +# feature (#303). They compared results with the cache on against the cache off +# and asserted the two were equal, and one of them claimed in a comment to +# exercise LRU eviction. All three passed against a cache whose only entry point +# had no callers, because a correctness-only comparison of two identical paths +# cannot fail. If a cache returns here, its tests have to assert a hit that is +# observable and an eviction that evicts. # --------------------------------------------------------------------------- # Fallback: aggregates and column types the vectorized path does not handle must