From 9b3ef333edad889aa879b8931e9644b1afda5806 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 7 Aug 2026 20:36:41 -0600 Subject: [PATCH 1/2] feat: cache the FSST keep/drop verdict with an age bound (#472) Deciding whether an FSST symbol table pays for itself costs a whole-corpus encode plus a compression pass, and the answer cannot be sampled: on a training prefix FSST can look 24% worse while over the whole column it is 23% better, an inversion no margin would make safe. So the question was asked once per column per row group, and for a column whose data does not change character that re-derived the same answer for the whole load. RE-MEASURED FIRST, because the issue's numbers pre-dated #155's distinct probe and fsst_min_gain_percent and it says so. Instrumented build, PG18.4, 2,000,000 rows, 20 row groups: md5(g) 5319 ms load 2482 ms deciding hurts 20/20 47% email-shaped 2081 ms load 843 ms deciding hurts 20/20 41% 'label-'||g%40 533 ms load 0 ms build skipped by #155 low-card->md5 3021 ms load 1269 ms deciding hurts 10/10 42% So 41 to 47 percent of a text load re-derives a constant, which agrees with #499's independent profile of the ingest shape. The low-cardinality row is the control: #155 already skips the build there and that path costs nothing. The verdict is cached on PgColumnarColumnDef, which is palloc0ed, so UNKNOWN is the natural zero and a fresh write state always asks once. The cache lives exactly as long as the write state, which is one statement. Nothing is persisted and no on-disk structure changes. pgcolumnar.fsst_verdict_reuse bounds it, default 16, 0 meaning ask every time. The setting exists because the byte-identical test arm needs an off switch to compare against, and it doubles as the escape hatch. THE TWO VERDICTS DO NOT SAVE THE SAME WORK, and the code says so rather than pretending. A reused HURTS skips the build AND the question, because the vectors then take their ordinary encoding, which is what a fresh HURTS would have produced. A reused HELPS still builds the table, because the table is trained on THIS row group's corpus and stored with the chunk, so reusing the table itself would change the stored bytes; only the question is skipped, and that is the expensive half. Measured in-suite: 1623 ms against 648 ms on the md5 shape, about 2.5x. THE RISK IS SILENCE, NOT SPEED. A stale verdict does not corrupt anything; it compresses worse, correctly, and nothing would notice. So the headline checks are byte equality of the stored chunks rather than load time, compared through the catalog's encoding descriptor, block codec and page length per chunk. A checksum of the relation file would have been useless: columnar pages carry LSNs, so two identical loads differ. FIVE OF SIX CANDIDATE CORPORA RETURN HURTS. md5, urls, emails, JSON and log paths all decline FSST; only prose keeps it. A suite built on the common case would have exercised one branch and let a wrongly cached HELPS through, so the fixtures are one of each, and the premise asserts they take different branches using the descriptor decoder from write_fsst_compressed.sh. THE AGE BOUND'S ASSERTION WAS WRONG THE FIRST TIME, and the measurement corrected it. I asserted that a bounded cache stores no more than an unbounded one. It stores MORE: 5778575 against 5628054. PgColumnarFsstHelpsCompressed keeps FSST only when the win clears fsst_min_gain_percent, so a marginal win is declined deliberately and a stale HELPS takes that margin back. Smaller, and still the wrong call, because the margin pays for decode. The check now measures distance from the uncached DECISION: bounded 0, unbounded 150521. Proved by removal: with only the reuse condition disarmed and every other line identical, the speedup disappears (1601 vs 1639 ms) and the age-bound premise fires with "the fixture does not change character", which is exactly what an absent cache should look like. Fingerprinted bede85aa fixed, 23d6f010 armed, bede85aa restored byte-identical. The byte-equality arms still pass when disarmed, correctly: they are safety checks, not feature checks. Eleven suites re-run clean, including differential and parallel_copy. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 21 +++ design/FSST_VERDICT_CACHE_PLAN.md | 102 ++++++++++++++ src/columnar.h | 17 +++ src/columnar_tableam.c | 16 +++ src/columnar_write_state.c | 74 ++++++++-- test/fsst_verdict_cache.sh | 219 ++++++++++++++++++++++++++++++ test/run_all_versions.sh | 1 + 7 files changed, 442 insertions(+), 8 deletions(-) create mode 100644 design/FSST_VERDICT_CACHE_PLAN.md create mode 100644 test/fsst_verdict_cache.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 00d4eb1..4f7da99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,27 @@ which was true until that script existed. ### Added +- `pgcolumnar.fsst_verdict_reuse` caches a column's FSST keep/drop verdict for a + bounded number of row groups (#472). Default 16; `0` asks every time, which is + the behaviour before this setting existed. + + Deciding whether an FSST symbol table pays for itself costs a whole-corpus + encode plus a compression pass, and the answer cannot be sampled: on a training + prefix FSST can look 24 percent worse while over the whole column it is 23 + percent better. So it was asked once per column per row group, and for a column + whose data does not change character that re-derived the same answer for the + whole load. Measured at 2,000,000 rows in 20 row groups: 2482 ms of a 5319 ms + `md5` load and 843 ms of a 2081 ms email-shaped load, with the verdict identical + all 20 times. + + A text load is about 2.5 times faster as a result, measured in-suite at 1623 ms + against 648 ms. Stored bytes are unchanged for a column whose verdict is stable, + which is asserted rather than assumed: the suite compares the encoding + descriptor, block codec and page length of every chunk. A column that changes + character mid-load is noticed within the bound. + + Reuse is per statement. Nothing is persisted and no on-disk structure changes. + - `EXPLAIN (ANALYZE)` now reports `Columnar Usable Skip Predicates` beside `Columnar Pushed-Down Filters` (#479). The existing line counts the filters the scan was handed and is unchanged; the new one counts how many of those the diff --git a/design/FSST_VERDICT_CACHE_PLAN.md b/design/FSST_VERDICT_CACHE_PLAN.md new file mode 100644 index 0000000..2047ea3 --- /dev/null +++ b/design/FSST_VERDICT_CACHE_PLAN.md @@ -0,0 +1,102 @@ +# Caching the FSST keep/drop verdict (#472) + +## Motivation, re-measured + +`pgcolumnar_flush_row_group` decides FSST keep/drop once per column per row +group. The decision cannot use a sample: on a 256 kB training prefix FSST can +look 24% worse while over the whole column it is 23% better, an inversion no +margin would make safe, so `PgColumnarFsstHelpsCompressed` FSST-encodes the +whole corpus and compresses it, only to answer a yes or no. + +For a column whose data does not change character, that recomputes the same +answer for every row group of the load. + +Measured on current `main` (PG18.4, 2,000,000 rows, `stripe_row_limit` 100000, +so 20 row groups), with the decision instrumented: + +| shape | load | decisions | verdict | deciding | share of load | +| --- | ---: | ---: | --- | ---: | ---: | +| `md5(g)` | 5319 ms | 20 | hurts 20/20 | 2482 ms | 47% | +| email-shaped | 2081 ms | 20 | hurts 20/20 | 843 ms | 41% | +| `'label-' \|\| (g%40)` | 533 ms | 20 | build skipped (#155) | 0 ms | 0% | +| low-card then md5 | 3021 ms | 10 built | hurts 10/10 | 1269 ms | 42% | + +So 41 to 47 percent of a text load re-derives a constant. This is consistent +with #499's independent profile of the ingest shape (`encode_fsst_shared` 33.9%, +`PgColumnarFsstBuildChunkTable` 14.6%). + +The low-cardinality row is the control: #155's `PgColumnarFsstDictWins` already +skips the build there, and that path costs nothing. This work is about the other +one. + +## Design + +Two fields on `PgColumnarColumnDef`, which is `palloc0`ed per write state, so +`UNKNOWN` is the natural zero: + +```c +#define COLUMNAR_FSST_UNKNOWN 0 +#define COLUMNAR_FSST_HELPS 1 +#define COLUMNAR_FSST_HURTS 2 + +int8 fsstVerdict; +int fsstVerdictAge; /* row groups since the verdict was taken */ +``` + +A write state lives for one statement, so the cache never outlives the load that +built it. Nothing is persisted and no on-disk format changes. + +`pgcolumnar.fsst_verdict_reuse`, integer, default 16, minimum 0. After a verdict +is taken it is reused for the next N row groups, then re-taken. **0 means never +reuse**, which is exactly today's behaviour and is what the byte-identical test +arm compares against. It is also the escape hatch. + +The two verdicts save different amounts, and the plan should not pretend +otherwise: + +- **HURTS reused**: skip the table build AND the decision. The vectors take + their ordinary encoding, which is what a fresh HURTS verdict would have done. +- **HELPS reused**: still build the table, because it is trained on this row + group's corpus and stored with the chunk, so reusing the TABLE would change + stored bytes. Only the whole-corpus decision is skipped. The build is the + cheaper half (14.6% against 33.9% in #499's profile). + +## What must be proven, and how + +The saving is real only if the stored bytes do not change. A stale verdict +silently degrades compression, and nothing in the current suites would notice. + +Fixtures, chosen by measurement rather than by assumption. Six candidate corpora +were tried and five of them return HURTS; a cache tested only on those would +exercise one branch and a wrongly cached HELPS would sail through: + +| fixture | verdict | purpose | +| --- | --- | --- | +| `md5(g)` | hurts, stable | the common case | +| `'the quick brown fox ... ' \|\| g \|\| ' in the morning'` | **helps, stable** | the other branch | +| prose for half the load, then `md5` | helps then hurts | the age bound | + +1. **Byte-identical storage, cache on against cache off**, on both stable + fixtures. `pg_total_relation_size` is too coarse to be the only check, so the + arm compares the relation's bytes. + +2. **The age bound is tested rather than assumed.** For a column that changes + character mid-load, caching cannot produce byte-identical output: within the + reuse window the stale verdict is used deliberately. So the assertions are: + - `fsst_verdict_reuse = 1` must be byte-identical to `0`, which pins the + mechanism: a bound of one row group re-decides every time. + - on the changing fixture, a bounded cache must land materially closer to the + uncached size than an effectively unbounded one. Without this the bound is + a number nobody checked. + +3. **Correctness is asserted separately from size.** Every arm reads its data + back and compares against a heap mirror. A compression regression is a cost; + a decode failure is a defect, and the two must not share one check. + +4. **The load-time win measured in-suite**, not from a standalone probe. + +## Risk + +Write-path change. The failure mode is silent: worse compression, correct data. +That is why proof 1 is byte equality rather than a size bound, and why proof 2 +exists at all. No format change, no read-path change, no catalog change. diff --git a/src/columnar.h b/src/columnar.h index 4321973..040073a 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -167,6 +167,23 @@ extern int pgcolumnar_encoding_sample_rows; extern int pgcolumnar_compression; /* one of COLUMNAR_COMPRESSION_* */ extern int pgcolumnar_compression_level; /* zstd level */ extern int pgcolumnar_fsst_min_gain_percent; /* min compressed FSST win to keep it (#155) */ +/* + * How many row groups may reuse a column's FSST keep/drop verdict before it is + * taken again (#472). 0 never reuses, which is the behaviour before that issue + * and what the byte-identical test arm compares against. + * + * Asking the question costs a whole-corpus FSST encode plus a compression pass, + * once per column per row group, because the answer cannot be sampled: on a + * training prefix FSST can look 24% worse while over the whole column it is 23% + * better. Measured at 41 to 47 percent of a text load, re-deriving a verdict + * that did not change across 20 row groups. + */ +extern int pgcolumnar_fsst_verdict_reuse; + +/* a column's cached FSST verdict (#472) */ +#define COLUMNAR_FSST_UNKNOWN 0 +#define COLUMNAR_FSST_HELPS 1 +#define COLUMNAR_FSST_HURTS 2 extern bool pgcolumnar_enable_qual_pushdown; extern bool pgcolumnar_enable_column_projection; extern bool pgcolumnar_enable_custom_scan; diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 3c506bf..70d8cc7 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -60,6 +60,7 @@ PG_MODULE_MAGIC; /* GUC-backed instance defaults (spec 8.3) */ int pgcolumnar_stripe_row_limit = 150000; int pgcolumnar_chunk_group_row_limit = 10000; +int pgcolumnar_fsst_verdict_reuse = 16; int pgcolumnar_encoding_sample_rows = 2048; int pgcolumnar_compression = COLUMNAR_COMPRESSION_ZSTD; @@ -2299,6 +2300,21 @@ _PG_init(void) 0, NULL, NULL, NULL); + DefineCustomIntVariable("pgcolumnar.fsst_verdict_reuse", + "Row groups that may reuse a column's FSST keep/drop verdict.", + "Deciding whether an FSST symbol table pays for itself costs a " + "whole-corpus encode plus a compression pass, and the answer " + "cannot be sampled, so it is asked once per column per row " + "group. For a column whose data does not change character that " + "re-derives a constant. 0 asks every time, which is the " + "behaviour before this setting existed.", + &pgcolumnar_fsst_verdict_reuse, + 16, + 0, INT_MAX, + PGC_USERSET, + 0, + NULL, NULL, NULL); + DefineCustomIntVariable("pgcolumnar.encoding_sample_rows", "Rows sampled to choose a chunk's value encoding.", "Candidate encodings are estimated on a windowed sample " diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index e7700f0..f884f27 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -104,6 +104,16 @@ typedef struct PgColumnarColumnDef bool bloomable; FmgrInfo hashFn; Oid hashCollation; /* collation to hash under (InvalidOid if none) */ + + /* + * Cached FSST keep/drop verdict and how many row groups have reused it + * (#472). COLUMNAR_FSST_UNKNOWN is the palloc0 default, so a fresh write + * state always asks once. The cache lives exactly as long as the write + * state, which is one statement: nothing is persisted and no on-disk + * structure changes. + */ + int8 fsstVerdict; + int fsstVerdictAge; } PgColumnarColumnDef; /* one column's two streams within one chunk group */ @@ -1046,6 +1056,8 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState) { StringInfoData corpus; uint32 sampleLen = 0; + PgColumnarColumnDef *def = &writeState->colDefs[c]; + bool reuseVerdict; initStringInfo(&corpus); foreach(lc, writeState->chunkGroups) @@ -1088,8 +1100,19 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState) * the keep/drop decision uses, and only skips when the dictionary is * viable and wins for every vector, so the stored bytes are identical. */ + /* + * Reuse this column's previous verdict when it is young enough + * (#472). A HURTS verdict skips the build as well as the question, + * since the vectors then take their ordinary encoding, which is + * what a freshly taken HURTS would have produced. + */ + reuseVerdict = (pgcolumnar_fsst_verdict_reuse > 0 && + def->fsstVerdict != COLUMNAR_FSST_UNKNOWN && + def->fsstVerdictAge < pgcolumnar_fsst_verdict_reuse); + if (corpus.len > 0 && writeState->encodeEffort != COLUMNAR_ENCODE_EFFORT_FAST && + !(reuseVerdict && def->fsstVerdict == COLUMNAR_FSST_HURTS) && !PgColumnarFsstDictWins(corpus.data, (uint32) corpus.len)) PgColumnarFsstBuildChunkTable(corpus.data, sampleLen, att, &fsstTable, &fsstTableLen); @@ -1111,15 +1134,50 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState) * the whole column it is 23% better -- a verdict that is not merely * imprecise but inverted, so no margin on the sample would be safe. */ - if (fsstTable != NULL && - !PgColumnarFsstHelpsCompressed(corpus.data, (uint32) corpus.len, - fsstTable, fsstTableLen, - writeState->compressionType, - writeState->compressionLevel)) + if (fsstTable != NULL) { - pfree(fsstTable); - fsstTable = NULL; - fsstTableLen = 0; + bool helps; + + /* + * A reused HELPS verdict still builds the table above, because + * the table is trained on THIS row group's corpus and stored + * with the chunk: reusing the table itself would change the + * stored bytes. Only the whole-corpus question is skipped, and + * that is the expensive half. + */ + if (reuseVerdict) + { + helps = (def->fsstVerdict == COLUMNAR_FSST_HELPS); + def->fsstVerdictAge++; + } + else + { + helps = PgColumnarFsstHelpsCompressed(corpus.data, + (uint32) corpus.len, + fsstTable, fsstTableLen, + writeState->compressionType, + writeState->compressionLevel); + def->fsstVerdict = helps ? COLUMNAR_FSST_HELPS + : COLUMNAR_FSST_HURTS; + def->fsstVerdictAge = 0; + } + + if (!helps) + { + pfree(fsstTable); + fsstTable = NULL; + fsstTableLen = 0; + } + } + else if (reuseVerdict && def->fsstVerdict == COLUMNAR_FSST_HURTS) + { + /* + * The build was skipped on the strength of the cached verdict, + * so this row group counts as a reuse too. Without this the age + * would never advance on the common path and the bound would + * never re-take the verdict. + */ + def->fsstVerdictAge++; } pfree(corpus.data); diff --git a/test/fsst_verdict_cache.sh b/test/fsst_verdict_cache.sh new file mode 100644 index 0000000..59f8db4 --- /dev/null +++ b/test/fsst_verdict_cache.sh @@ -0,0 +1,219 @@ +#!/usr/bin/env bash +# +# pgColumnar: the FSST keep/drop verdict is cached with an age bound (#472). +# +# pgcolumnar_flush_row_group decides FSST keep/drop once per column per row +# group, and the decision cannot use a sample: on a training prefix FSST can +# look 24% worse while over the whole column it is 23% better, an inversion no +# margin would make safe. So PgColumnarFsstHelpsCompressed FSST-encodes the +# whole corpus and compresses it, only to answer yes or no. For a column whose +# data does not change character, that recomputes the same answer for every row +# group of the load. +# +# Measured on main before this change, 2,000,000 rows in 20 row groups: 2482 ms +# of a 5319 ms md5 load and 843 ms of a 2081 ms email load went to deciding, and +# the verdict was the same all 20 times. 41 to 47 percent of a text load +# re-deriving a constant. +# +# THE RISK IS NOT SPEED, IT IS SILENCE. A stale verdict does not corrupt +# anything; it compresses worse, correctly, and nothing would notice. So the +# headline checks here are byte equality of the stored chunks, not load time. +# +# Usage: test/fsst_verdict_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_FSST_ROWS:-600000} +NGROUPS=6 # ROWS / stripe_row_limit below. + # NOT `GROUPS`: that is a bash special + # variable holding the caller's group ids, + # so assigning it silently does nothing and + # $((ROWS / NGROUPS)) divides by zero. + +# Two corpora, chosen by measurement rather than by assumption. Six candidates +# were tried and five return HURTS (md5, urls, emails, JSON, log paths); prose +# is the only one that returns HELPS. A suite that tested only the common case +# would exercise one branch, and a wrongly cached HELPS verdict would sail +# through it. +PROSE="'the quick brown fox jumps over the lazy dog number ' || g || ' in the morning'" +MD5="md5(g::text)" + +# Load with at a given pgcolumnar.fsst_verdict_reuse, and leave +# the elapsed milliseconds in LOAD_MS. +LOAD_MS=0 +load() { # load
+ local tbl="$1" expr="$2" reuse="$3" t0 t1 + psql_run "DROP TABLE IF EXISTS $tbl; + CREATE TABLE $tbl (t text) USING pgcolumnar; + SELECT pgcolumnar.set_options('$tbl', stripe_row_limit => $((ROWS / NGROUPS)));" >/dev/null + t0=$(date +%s%N) + psql_run "SET pgcolumnar.fsst_verdict_reuse = $reuse; + INSERT INTO $tbl SELECT $expr FROM generate_series(1,$ROWS) g;" >/dev/null + t1=$(date +%s%N) + LOAD_MS=$(( (t1 - t0) / 1000000 )) +} + +# The stored bytes, as a fingerprint that does not move with the LSN. +# +# A checksum of the relation file would be useless here: columnar pages live in +# the main fork and carry page headers, so two identical loads differ in their +# LSNs and every comparison below would fail for a reason that is not the one +# under test. The catalog records exactly what the encoder chose and how long +# the result was, which is the property this change must not alter. +fingerprint() { # fingerprint
+ q "SELECT md5(string_agg( + c.group_number || ':' || c.column_index || ':' || c.value_count || ':' || + encode(c.encoding_descriptor, 'hex') || ':' || c.block_codec || ':' || + c.page_length, '|' ORDER BY c.group_number, c.column_index)) + FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = '$1'::regclass;" +} + +# How many vectors chose FSST. Lifted from test/write_fsst_compressed.sh, whose +# comment carries the trap: the descriptor is a 6-byte header (version, a +# reserved byte, then the vector count as uint32) followed by that many 13-byte +# entries and then the chunk-shared symbol table, so entry i's type byte is at +# 6 + i*13 and the count must come from the header rather than from the length. +# Reading past the entries scores the symbol table's own bytes as encoding types. +fsst_vectors() { # fsst_vectors
+ q "SELECT coalesce(sum(n), 0) FROM ( + SELECT (SELECT count(*) + FROM generate_series(0, + get_byte(c.encoding_descriptor, 2) + + get_byte(c.encoding_descriptor, 3) * 256 + + get_byte(c.encoding_descriptor, 4) * 65536 + + get_byte(c.encoding_descriptor, 5) * 16777216 - 1) i + WHERE get_byte(c.encoding_descriptor, 6 + i * 13) = 8) AS n + FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = '$1'::regclass) t;" | tail -1 +} + +# An empty count is not a zero count: `[ "" -gt 0 ]` errors instead of being +# false, and the check would then report on a number that was never read. +fsst_or_none() { local v; v="$(fsst_vectors "$1")"; pgc_is_number "$v" && echo "$v" || echo none; } + +rows_of() { q "SELECT count(*) FROM $1;"; } +groups_of() { q "SELECT count(DISTINCT group_number) FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = '$1'::regclass;"; } + +# --- 0. the premises, before any comparison is believed ------------------------- + +load fv_prose_off "$PROSE" 0 +check_num "premise: the prose fixture loaded" "$(rows_of fv_prose_off)" "$ROWS" +check "premise: and it spans several row groups, so caching has something to reuse" \ + "$([ "$(groups_of fv_prose_off)" -ge 3 ] && echo yes || echo "no ($(groups_of fv_prose_off))")" "yes" + +load fv_md5_off "$MD5" 0 +check_num "premise: the md5 fixture loaded" "$(rows_of fv_md5_off)" "$ROWS" + +# The premise that stops this suite testing one branch twice. Without it, both +# fixtures could be taking the same path and every equality below would still +# hold, which is the shape of a green suite that measures nothing. +check "premise: the prose corpus KEEPS fsst, so the HELPS branch is exercised" \ + "$(v=$(fsst_or_none fv_prose_off); [ "$v" != none ] && [ "$v" -gt 0 ] && echo yes || echo "no ($v)")" "yes" +check "premise: and the md5 corpus DROPS it, so the two arms are different branches" \ + "$(fsst_or_none fv_md5_off)" "0" + +# --- 1. a cached verdict must not change one stored byte ------------------------ + +load fv_prose_on "$PROSE" 16 +check_text "a reused HELPS verdict stores byte-identical chunks" \ + "$(fingerprint fv_prose_on)" "$(fingerprint fv_prose_off)" +check "and it still keeps fsst, rather than silently dropping it" \ + "$(v=$(fsst_or_none fv_prose_on); [ "$v" != none ] && [ "$v" -gt 0 ] && echo yes || echo "no ($v)")" "yes" + +load fv_md5_on "$MD5" 16 +check_text "a reused HURTS verdict stores byte-identical chunks" \ + "$(fingerprint fv_md5_on)" "$(fingerprint fv_md5_off)" +check "and it still declines fsst" "$(fsst_or_none fv_md5_on)" "0" + +# --- 2. the data, which is a different question from the bytes ------------------ +# +# A compression regression is a cost; a decode failure is a defect. They must +# not share a check, so the values are compared independently of the chunks. +psql_run "DROP TABLE IF EXISTS fv_heap; + CREATE TABLE fv_heap (t text); + INSERT INTO fv_heap SELECT $PROSE FROM generate_series(1,$ROWS) g;" >/dev/null +check "the cached load returns the same values as heap" \ + "$(pgc_set_hash 'SELECT t FROM fv_prose_on')" \ + "$(pgc_set_hash 'SELECT t FROM fv_heap')" + +# --- 3. the age bound, tested rather than assumed ------------------------------- +# +# A bound of one row group must re-decide every time, so it has to reproduce the +# uncached bytes exactly. This is what pins the mechanism: if the age were +# ignored, or off by one, this is the check that moves. +load fv_prose_one "$PROSE" 1 +check_text "a reuse bound of one row group is byte-identical to no caching" \ + "$(fingerprint fv_prose_one)" "$(fingerprint fv_prose_off)" + +# And a column that changes character mid-load. Caching CANNOT be byte-identical +# here, because within the window the stale verdict is used deliberately, so the +# assertion is the one that matters: a bounded cache notices the change and an +# unbounded one does not. +CHANGING="CASE WHEN g <= $((ROWS / 2)) THEN $PROSE ELSE $MD5 END" +load fv_chg_off "$CHANGING" 0 +load fv_chg_bounded "$CHANGING" 2 +load fv_chg_unbounded "$CHANGING" 1000000 + +chg_off="$(q "SELECT sum(page_length) FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = 'fv_chg_off'::regclass;")" +chg_bounded="$(q "SELECT sum(page_length) FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = 'fv_chg_bounded'::regclass;")" +chg_unbounded="$(q "SELECT sum(page_length) FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = 'fv_chg_unbounded'::regclass;")" +echo "-- changing column stored bytes: uncached=$chg_off bounded=$chg_bounded unbounded=$chg_unbounded" + +check "premise: the three changing-column loads all produced bytes to compare" \ + "$(if pgc_is_number "$chg_off" && pgc_is_number "$chg_bounded" \ + && pgc_is_number "$chg_unbounded"; then echo yes; else echo no; fi)" "yes" + +# WHICH DIRECTION IS "BETTER" IS NOT THE OBVIOUS ONE, and assuming it is what +# this check got wrong first. A stale HELPS verdict can store FEWER bytes than +# the correct decision: PgColumnarFsstHelpsCompressed keeps FSST only when the +# compressed win clears pgcolumnar.fsst_min_gain_percent, so a marginal win is +# declined deliberately, and forcing FSST through a stale verdict takes that +# margin back. Measured here: uncached 5778575, unbounded 5628054. Smaller, and +# still the wrong call, because the margin exists to pay for decode. +# +# So the assertion is about tracking the uncached DECISION, not about size. +_d_bounded=$(( chg_bounded > chg_off ? chg_bounded - chg_off : chg_off - chg_bounded )) +_d_unbounded=$(( chg_unbounded > chg_off ? chg_unbounded - chg_off : chg_off - chg_unbounded )) +echo "-- distance from the uncached decision: bounded=$_d_bounded unbounded=$_d_unbounded" + +# Without this the comparison below could hold with both distances zero, which +# is what a fixture that does not actually change character would produce. +check "premise: an unbounded cache really does diverge on this fixture" \ + "$([ "$_d_unbounded" -gt 0 ] && echo yes || echo "no (the fixture does not change character)")" "yes" + +check "a bounded cache tracks the change more closely than an unbounded one" \ + "$([ "$_d_bounded" -lt "$_d_unbounded" ] && echo yes || echo "no ($_d_bounded vs $_d_unbounded)")" "yes" + +check "and the changing column still returns its values" \ + "$(q "SELECT count(*) FROM fv_chg_bounded;")" "$ROWS" + +# --- 4. the win, in this suite rather than from a standalone probe -------------- +# +# Deliberately loose. The measured saving on this shape is 40 percent and more, +# so a 10 percent bound fails only if the caching stopped working, not because +# the box is busy. A tighter bound would buy nothing and would flake. +load fv_time_off "$MD5" 0 +t_off=$LOAD_MS +load fv_time_on "$MD5" 16 +t_on=$LOAD_MS +echo "-- md5 load: uncached=${t_off} ms cached=${t_on} ms" +check "caching the verdict makes the load measurably faster" \ + "$([ "${t_on:-0}" -lt "$(( ${t_off:-0} * 90 / 100 ))" ] && echo yes || echo "no ($t_on vs $t_off ms)")" \ + "yes" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index e814d44..8a72011 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -67,6 +67,7 @@ SUITES=( encode_invariants fk_referencing fsst_margin + fsst_verdict_cache fuzz fuzz_arrow fuzz_parquet From 827d1b0c3b69865797bd9533aab14f00f13a6cd8 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 05:58:18 -0600 Subject: [PATCH 2/2] fix: do not shadow the enclosing def (#472 review) CI builds with -Wshadow=compatible-local and fails on any compiler warning, so this reddened seven of ten build legs: src/columnar_write_state.c:1059:46: warning: declaration of 'def' shadows a previous local [-Wshadow=compatible-local] The declaration was not merely shadowing, it was redundant: the enclosing per-column block already binds `def` to `&writeState->colDefs[c]`, the same object. Removed, with a comment naming the flag so the next person does not reintroduce it. WHY MY LOCAL LOOP DID NOT CATCH IT. I built with `make ... >/dev/null || { grep warning ...; }`, so warnings were only ever printed when make FAILED, and a warning does not fail make. CI greps build.err unconditionally. Rebuilt all five majors with stderr kept and warnings counted: 0 on 15, 16, 17, 18 and 19. PG15 passed CI while 16 through 19 failed, which is worth knowing: the warning is gcc-version dependent, so one green major says nothing about the others for this class. Co-Authored-By: Claude Opus 5 (1M context) --- src/columnar_write_state.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index f884f27..959590b 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -1056,7 +1056,9 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState) { StringInfoData corpus; uint32 sampleLen = 0; - PgColumnarColumnDef *def = &writeState->colDefs[c]; + /* `def` is the enclosing block's, at the top of this per-column + * loop. Re-declaring it here shadowed that one, which this project + * builds with -Wshadow=compatible-local and treats as an error. */ bool reuseVerdict; initStringInfo(&corpus);