Found while working #282, which asked why columnar_cache.c sits at 6.7% line and
2.8% branch coverage. The answer is not that it is badly tested. It is that the
cache never runs.
Evidence
columnar_cache.c exposes exactly two non-static functions:
| function |
callers outside the file |
ColumnarCacheInit |
1, from _PG_init at columnar_tableam.c:2383 |
ColumnarGetDecompressedStream |
0 |
ColumnarGetDecompressedStream is the only way to put anything in the cache or
get anything out of it. Nothing calls it. What executes is the init path and
nothing else, which is exactly the 6.7%.
It was wired once
c592941 Phase 6: vectorized scan and aggregates, decompressed-chunk cache
881fa51 Phase H2: remove the 1.0-dev (2.2) on-disk format, catalog, and selector
The cache was introduced with a caller in Phase 6. Phase H2 removed the old
on-disk format and took the call site with it. That looks unintended: H2's subject
is the format removal, and nothing in it is about the cache.
Why no test caught it
This is the part worth dwelling on. test/phase6.sh has three cache assertions,
and all three pass on a cache that does not exist:
# Run a query with the decompressed-chunk cache on and off; assert equality.
cache_on_off() { ... assert on == off ... }
The property asserted is that results are identical with the cache on and off.
For a dead cache that is trivially true. The third is worse, because its comment
states an intent the code cannot deliver:
# 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;")"
It exercises no eviction. columnar_lru_evict has never run.
A correctness-only assertion cannot distinguish a working cache from a removed
one. That is the same shape as an empty REGRESS reporting success, and as the
suites that reported PASS while skipping themselves.
What is currently claimed
Two GUCs that do nothing, and documentation that describes behaviour the code does
not have:
pgcolumnar.enable_column_cache, pgcolumnar.column_cache_size
docs/configuration.md: "Cache decompressed chunk groups so they can be reused
across reads"
docs/administration.md: "keeps chunk groups after decompression. Other reads
can then use them again"
docs/ARCHITECTURE.md describes the cache and its bound
docs/testing.md says phase6.sh covers "the column cache"
A user can set both GUCs, read the documentation, and reasonably believe they have
enabled something.
Two ways out, and this is a product decision
-
Re-wire it. Restore a call site in the native read path. This is not a
revert: H2 replaced the format the old caller belonged to, so the correct
insertion point in the current reader has to be worked out, and the
performance case has to be re-made. The cache is off by default, so this is not
urgent, but it is real work.
-
Remove it. Delete columnar_cache.c, both GUCs, and the documentation
claims. Small, honest, and it stops the project claiming a capability it does
not have.
I lean to 2 unless someone wants to make the performance case for 1, on the
grounds that a feature which has been dead since 22 July without anyone noticing
is not one users are relying on. Whichever is chosen, the phase6.sh assertions
need to become ones that can fail: an equality check between two paths that are
the same path proves nothing.
Not urgent and nothing is broken for users, since the cache is off by default and
does nothing when switched on. The harm is the false claim, and a coverage report
that reads as "badly tested" when it should read as "not connected".
Found while working #282, which asked why
columnar_cache.csits at 6.7% line and2.8% branch coverage. The answer is not that it is badly tested. It is that the
cache never runs.
Evidence
columnar_cache.cexposes exactly two non-static functions:ColumnarCacheInit_PG_initatcolumnar_tableam.c:2383ColumnarGetDecompressedStreamColumnarGetDecompressedStreamis the only way to put anything in the cache orget anything out of it. Nothing calls it. What executes is the init path and
nothing else, which is exactly the 6.7%.
It was wired once
The cache was introduced with a caller in Phase 6. Phase H2 removed the old
on-disk format and took the call site with it. That looks unintended: H2's subject
is the format removal, and nothing in it is about the cache.
Why no test caught it
This is the part worth dwelling on.
test/phase6.shhas three cache assertions,and all three pass on a cache that does not exist:
The property asserted is that results are identical with the cache on and off.
For a dead cache that is trivially true. The third is worse, because its comment
states an intent the code cannot deliver:
It exercises no eviction.
columnar_lru_evicthas never run.A correctness-only assertion cannot distinguish a working cache from a removed
one. That is the same shape as an empty
REGRESSreporting success, and as thesuites that reported PASS while skipping themselves.
What is currently claimed
Two GUCs that do nothing, and documentation that describes behaviour the code does
not have:
pgcolumnar.enable_column_cache,pgcolumnar.column_cache_sizedocs/configuration.md: "Cache decompressed chunk groups so they can be reusedacross reads"
docs/administration.md: "keeps chunk groups after decompression. Other readscan then use them again"
docs/ARCHITECTURE.mddescribes the cache and its bounddocs/testing.mdsaysphase6.shcovers "the column cache"A user can set both GUCs, read the documentation, and reasonably believe they have
enabled something.
Two ways out, and this is a product decision
Re-wire it. Restore a call site in the native read path. This is not a
revert: H2 replaced the format the old caller belonged to, so the correct
insertion point in the current reader has to be worked out, and the
performance case has to be re-made. The cache is off by default, so this is not
urgent, but it is real work.
Remove it. Delete
columnar_cache.c, both GUCs, and the documentationclaims. Small, honest, and it stops the project claiming a capability it does
not have.
I lean to 2 unless someone wants to make the performance case for 1, on the
grounds that a feature which has been dead since 22 July without anyone noticing
is not one users are relying on. Whichever is chosen, the
phase6.shassertionsneed to become ones that can fail: an equality check between two paths that are
the same path proves nothing.
Not urgent and nothing is broken for users, since the cache is off by default and
does nothing when switched on. The harm is the false claim, and a coverage report
that reads as "badly tested" when it should read as "not connected".