Remove the dead column cache (#303) - #308
Merged
Merged
Conversation
Collaborator
|
Reviewed and verified on pg18a (assert). The removal is clean and the reasoning is right — this is dead code from the H2 format change, not a wire to reconnect. Verified:
One accuracy fix for the body/CHANGELOG. The removed GUC doesn't reject startup. With So "must drop the line, or the server will reject an unrecognised |
jdatcmd
force-pushed
the
fix/303-remove-dead-column-cache
branch
from
July 31, 2026 22:34
ad99145 to
8730b6c
Compare
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) <noreply@anthropic.com>
jdatcmd
force-pushed
the
fix/303-remove-dead-column-cache
branch
from
July 31, 2026 22:45
8730b6c to
0ad4ed4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #303 and #282. Five-major matrix: ALL VERSIONS PASSED.
The decision
Removed rather than re-wired, and the argument is not "it is unused". It is that
this is not a wire to reconnect. The code was written against the on-disk
format Phase H2 deleted; the current native reader decompresses on a different
path. Restoring the behaviour means writing a new integration and making the
performance case again from scratch. Its value now is as a design sketch, which
the git history keeps. #289's vectorized decompression work would shape any future
caching layer far more than this code would.
What was actually wrong
ColumnarGetDecompressedStreamhad zero callers since 22 July. What ran wasColumnarCacheInitfrom_PG_init, which is the entire 6.7% line coverage #282asked about.
Two GUCs and four documentation passages described a feature that did nothing. A
user could set
pgcolumnar.enable_column_cache, read the administration guide,and reasonably believe they had enabled something.
The tests deserve naming
phase6.shhad three cache assertions and all three passed on a dead cache.They compared results with the cache on against the cache off and asserted the two
were equal, which is trivially true when both are the same path. One claimed in
its comment to "exercise LRU eviction";
columnar_lru_evicthas never run.Removed with the feature, and the comment left in their place says what a
replacement would have to assert: a hit that is observable, and an eviction that
evicts. A correctness-only comparison cannot distinguish a working cache from an
absent one.
User-visible
A
postgresql.confthat sets either parameter must drop the line, or the serverwill reject an unrecognised
pgcolumnar.*parameter. Recorded in the CHANGELOGunder a new Removed heading. Pre-release, no compatibility guarantee, and
neither setting did anything.
Scope
src/columnar_cache.cdeleted, both GUCs and the header block removed, theMakefile object dropped,
phase6.shassertions removed, andconfiguration.md,administration.md,ARCHITECTURE.mdandtesting.mdcorrected. Docs pass the style gate.