Size a bloom filter by its distinct count, not its value count (#467)

[jdatcmd]

Closes #467. Two commits: the red test, then the fix.

Measured

Red fixture, 20,000 rows in one row group, PG17:

before   lo (5 distinct) = 32773    hi (20000 distinct) = 32773
after    lo (5 distinct) =    13    hi (20000 distinct) = 32773

13 bytes is the 5-byte header plus next_pow2(5 * 10) = 64 bits. The high-cardinality column is untouched — this removes waste, it does not trade accuracy for size.

The before row also confirms your arithmetic independently: next_pow2(20000 * 10) = 262,144 bits = 32,768 bytes + header.

Two guards, moved differently, on purpose

n < 64 stays on the value count. It means "this chunk is too small to be worth a filter", which is about the chunk, not its cardinality. Moving it to the distinct count drops the filter entirely from all 68 of your 105 low-cardinality columns — and those are where an equality probe skips best: if the value is not among the five present, the whole group goes. A 64-bit filter is 8 bytes.

This is the sub-decision I flagged on the issue rather than taking silently. You may still overrule it — it is one line and the suite would need a different assertion for the low-cardinality column, nothing more.

BLOOM_MAX_BITS moves to the distinct count. The saturation it guards against is a function of how many distinct values compete for the bits, not how many times they repeat. Testing n refused a filter to a large stripe of low-cardinality data for a problem that data does not have.

The test, and what stops it lying

test/bloom_sizing.sh, 10 checks, registered inside SUITES and verified by sourcing the array rather than reading the diff.

It asserts the relation between two cardinalities in one table, not a literal byte count, so it will not fail the next time BLOOM_BITS_PER_VALUE is tuned.

Three arms passed before the fix and must still pass after: an absent value is excluded, a present value is found, and results are identical with the filter and without it. Those exist because shrinking a filter by making it useless would satisfy every size check in the suite. That is the failure a size-only test calls a success.

Six premises run before any size is compared, including that both filter rows exist — octet_length of a missing row is empty, not 0, and an empty side compares equal to another empty side (#418).

Do not "fix" the compression ratio later

This will make pglz look worse on pgcolumnar.bloom.filter, and that is correct. Your SET STORAGE EXTERNAL ablation settled that pglz taking 361 MB to 58 MB is a good trade and must not be replaced by changing default_toast_compression or the column storage.

Restating your own conclusion in the commit because it is counter-intuitive and someone will re-profile this: the 6.2x ratio was the symptom. Filters compressed that well because they were nearly all zeros. Correctly sized filters compress far less — that is the waste leaving, not a regression appearing.

Regressions

PG17: native_bloom, bloom_setting, bloom_lazy, native_skip, native_vecskip, pushdown_report, and differential (201 checks).

Not yet run on the full PG15-19 matrix; happy to before merge if you want it, as I did for #459 and #460.

@ChronicallyJD for review.

[jdatcmd]

Ran a high-effort multi-agent review against this branch before asking you to look at it. 22 candidate findings verified, 5 refuted, 7 kept — and the worst one is a regression I introduced while arguing the opposite on the issue. Posting them before I fix anything, because a clean PR would hide what was one merge away.

1. The BLOOM_MAX_BITS move ERRORs on large stripes (confirmed)

I moved the refusal to the distinct count and defended that on the issue. What I missed is that the refusal now sits after the dedup, so the allocation the old guard existed to avoid happens first.

With stripe_row_limit above 67,108,864 — a PGC_USERSET GUC whose max is INT_MAX, and the exact case the old guard was written for — cap rounds to 1<<28, palloc0(cap * 4) requests 1,073,741,824 bytes, one byte over MaxAllocSize, and the load aborts with invalid memory alloc request size. On main that load succeeds. Below the threshold it is a silent tax instead: at 1,000,000 rows per stripe it allocates and memsets 8 MB, walks 1M hashes, then discards the filter anyway — per bloomable column per stripe.

My commit message says the old guard "refused a filter to a large stripe of low-cardinality data for a problem that data does not have". That is still true about the sizing. I just moved the allocation in front of the escape hatch.

2. My own test header is wrong (confirmed)

I wrote that shrinking a filter by making it useless "would satisfy every size check here" and claimed the selectivity arm guarded against it. It does not. enable_bloom_filter = off gates only the read-side probe: filters are still written, sizes still differ 13 vs 32773, both selectivity checks still return their expected values, and the ON/OFF md5 pair is trivially equal because neither side used a filter. All ten checks pass with no bloom probe ever consulted.

native_bloom.sh has a real oracle — Columnar Chunk Groups Removed by Filter from EXPLAIN ANALYZE. Mine has none, and my own "the whole table is one row group" premise forecloses one.

3. ${LO:-0} defeats the missing-measurement guard (confirmed)

awk "BEGIN{print (${LO:-0} * 8 <= ${HI:-0}) ? 1 : 0}" — with both filters absent that is 0 <= 0, prints 1, and check_num compares 1 with 1 and passes. check_num is validating awk's verdict, not the byte counts. It is the #418 shape, four lines below a comment I wrote about avoiding the #418 shape. Red today only because unrelated premises fail first.

4. The suite never crosses the threshold that changed (confirmed)

ROWS=20000. The refusal fires above 209,715 distinct. The largest stripe_row_limit in any bloom suite is 20000 (bloom_lazy.sh:151); native_bloom.sh uses 2048. Both the old and the new refusal are dead branches under the full 15-19 matrix. The removal proof I would have cited goes red for the sizing formula alone, never for the guard — which is precisely why finding 1 would have shipped green.

5, 6, 7 (two plausible, one confirmed)

• All-unique columns (surrogate key, timestamp, uuid) pay the full O(n) cache-missing pass and arrive at d == n, same filter as before, no size win. Not covered by any check.
• The source comment states your four measurements as fact with nothing in the repo able to reproduce them.
• bloom_distinct_count re-implements what PostgreSQL ships as lib/hyperloglog.h.

What I am doing about it

Switching to lib/hyperloglog.h, which kills 1, 5 and 7 together. Fixed ~1 KB state regardless of n, so no MaxAllocSize ceiling and no allocation ahead of the guard, no per-column table walk, and 50 lines of hand-rolled open addressing leave the tree.

d becomes an estimate, and I think that is fine here specifically: next_pow2 quantizes to powers of two, so a few percent of error can only move the chosen size by one power of two, and it can never produce a false negative — a slightly-small filter raises the false-positive rate, costing a wasted decode rather than a wrong answer. Tell me if you read that differently, because it is the one place this stops being exact and it is load-bearing.

2 and 4 are one fixture change: a second table with multiple row groups, a real skip oracle from EXPLAIN ANALYZE, and an arm that actually crosses 209,715 distinct. 3 is two lines. For 6 I will keep your measurements in the commit message, attributed, and cut them from the source comment where they read as reproducible.

[ChronicallyJD]

Reviewed. The n < 64 call is right and I am not overruling it — I would have made the
same one. Two things verified rather than taken on trust, one of which is a portability
check you could not have run on a single major.

lib/hyperloglog.h across the matrix

It is a backend internal rather than a documented extension API, so presence and signatures
across 15 to 19 are worth confirming before this merges. Checked at each major's real
includedir-server:

major	initHyperLogLogError	addHyperLogLog	estimateHyperLogLog	freeHyperLogLog
15 / 16 / 17 / 18 / 19	ok	ok	ok	ok

Signatures are byte-identical between 15 and 19 — no drift:

extern void   initHyperLogLogError(hyperLogLogState *cState, double error);
extern void   addHyperLogLog(hyperLogLogState *cState, uint32 hash);
extern double estimateHyperLogLog(hyperLogLogState *cState);
extern void   freeHyperLogLog(hyperLogLogState *cState);

So the header choice is safe. (My first probe looked under include/server/ and reported it
missing on all five — that was my path being wrong, not yours. pg_config --includedir-server
is include/postgresql/server.)

n < 64 on the value count: agreed

Your reasoning holds and the adversarial case does not overturn it. The guard means "this
chunk is too small to warrant a filter", which is about the chunk rather than its
cardinality, and moving it would strip filters from 68 of 105 columns — the ones where an
equality probe skips best, since a value absent from the five present eliminates the whole
group.

The case against would be a low-cardinality column whose values are spread across every
group — a status column in unsorted data, where every stripe holds all five values and the
filter never skips anything. That filter is now 8 bytes rather than 256 KB, and the same
column in clustered data skips well. Cheap enough to keep for the upside.

The hyperLogLog argument checks out

Under-estimating d gives a smaller filter and a higher false positive rate, never a
false negative, because every value still gets its k bits set whatever nbits came out.
The worst outcome is decoding a group that could have been skipped — a performance cost,
bounded by HLL's 1.6% standard error and then quantised away by next_pow2.

Worth calling out that you found and fixed a real bug on the way: the exact-count first
version allocating past MaxAllocSize above 67,108,864 values in a group, on exactly the
large stripes the BLOOM_MAX_BITS refusal exists to short-circuit. Fixed state removes that
rather than reordering around it, which is the better fix.

BLOOM_MAX_BITS moving to the distinct count is right for the same reason saturation is
about how many distinct values compete for bits, not how often they repeat.

The three unchanged-behaviour arms are the important part of that suite

Absent value excluded, present value found, results identical with and without the filter —
passing before the fix and required to pass after. Shrinking a filter by making it useless
satisfies every size check, and a size-only suite would call that a success. That is the
assertion I would have asked for if it were not already there.

Asserting the relation between two cardinalities rather than a literal byte count is also
the right choice; it survives the next BLOOM_BITS_PER_VALUE tuning.

One request before merge

The full 15 to 19 matrix, as you offered. This touches the write path and the read path,
and our bar is full suites on 18 and 19 with preflight on all five. The HLL header is fine
across majors by inspection, but inspection is not a build.

Thanks for the note about not "fixing" the compression ratio later. That is the
counter-intuitive part and it will absolutely be re-profiled by someone: the 6.2x pglz ratio
was the symptom, and correctly sized filters compressing less is the waste leaving rather
than a regression arriving.

Approving on the matrix result.

[jdatcmd]

Full PG15-19 matrix, green, on the post-review branch (1017084).

PASS   PG15  (120 ran, 3 skipped)     bloom_sizing=PASS
PASS   PG16  (120 ran, 3 skipped)     bloom_sizing=PASS
PASS   PG17  (120 ran, 3 skipped)     bloom_sizing=PASS
PASS   PG18  (121 ran, 2 skipped)     bloom_sizing=PASS
PASS   PG19  (123 ran, 0 skipped)     bloom_sizing=PASS

No =FAIL on any suite on any major. Skips are the usual ones — temporal below PG18 for btree_gist, pg19_vacuum_options and native_repack below 19.

Two things I checked rather than assumed, because ALL VERSIONS PASSED is producible by a run that tested the wrong tree:

• the staged tree really is this branch — hyperloglog present in the source, the on/off skip oracle present in the suite, bloom_sizing confirmed inside the SUITES array by sourcing it
• bloom_sizing=PASS appears exactly five times, once per major, so it ran everywhere rather than being skipped somewhere

That matters here more than usual: the hyperLogLog path had only ever executed on PG17, and it runs per bloomable column per stripe on the write path, so a difference on another major would have shown up on every table at once. The 300,000-row arm that crosses the refusal threshold also ran on all five, which is new coverage — no bloom suite in the tree had ever crossed it.

Ready for review. The judgement I would most like you to push on is the estimate: d is now hyperLogLog at 1.6% standard error rather than an exact count. My argument is that next_pow2 quantises to powers of two so the error almost never changes the size chosen, and that under-estimating raises the false-positive rate — a wasted decode — while false negatives remain impossible because every value still sets its k bits whatever nbits came out. If you read that differently, it is the one place this stops being exact.

[ChronicallyJD]

Matrix is convincing, and the two things you checked rather than assumed are the right two —
bloom_sizing=PASS appearing exactly five times is what rules out the silent-skip case, and
the hyperLogLog path having previously run only on PG17 is exactly the kind of write-path
code where a per-major difference would surface on every table at once.

Approving. One piece of arithmetic on the judgement you asked me to push on, non-blocking,
because I think it is a follow-up rather than a change to this PR.

The estimate: your reasoning holds, and here is where it is thinnest

Agreed on the part that matters: false negatives stay impossible, because every value
sets its k bits whatever nbits came out, so a wrong d can only cost a wasted decode. That
is the difference between a performance question and a correctness one, and this is firmly
the former.

Where it is thinnest is the quantisation argument. next_pow2 usually absorbs the error, but
when it does not the step is not small:

k=6, 10 bits/value (intended)          FP = 0.84%
k=6,  5 bits/value (one power low)     FP = 11.6%     ~14x worse

BLOOM_K is fixed at 6, which is chosen for ~10 bits/value. Dropping one power of two
halves bits-per-value and k is then far from optimal for the new size, so the FP rate does
not degrade gently — it goes 0.84% to 11.6%.

For that to happen, d * 10 has to land in the ~2.3% of log-uniform range just below a power
of two and HLL has to under-estimate. So roughly 1% of bloomable column-stripes, each
getting a filter that fails to skip about one probe in nine instead of one in 119.

Why I still think it ships as-is

The comparison is not against a perfect filter, it is against what we had this morning: every
column at 256 KB, 19.3x over-provisioned. Trading ~1% of column-stripes into a higher FP rate
for that is clearly the right side of the deal, and the failure mode is a decode we could have
skipped rather than a wrong answer.

The follow-up, if you want it

Two obvious mitigations, and they pull against each other, which is why I would not bolt
either on here:

• Round the estimate up before next_pow2 (say d * 1.05). Removes nearly all of the
  under-shoot, but pushes some columns that were correctly sized up a power of two — paying
  2x space on those to protect ~1% of the others.
• Pick k from the bits-per-value actually allocated rather than fixing it at 6. That
  removes the cliff instead of avoiding it: at 5 bits/value the optimal k is ~3, which gives
  FP ~14% -> ~5%. It is a bigger change and it alters filters for every column, so it wants
  its own PR and its own skip-rate measurement.

The second is the more principled one and it is orthogonal to this — BLOOM_K was already
fixed at 6 while nbits moved with n, so the mismatch predates #467. Worth an issue rather
than scope on this.

Approved on the matrix.

[ChronicallyJD]

Approved, and CI-green — but this went CONFLICTING when I merged #462 a few minutes ago.
Same SUITES=(...) footgun you warned me about on my own PR, now pointing the other way.
Reciprocating your courtesy: flagging rather than pushing to your branch.

#462 added parallel_degree; this adds bloom_sizing to the same backslash-continued
line. Current main has 125 entries ending in isolation.

Your own verification recipe, which is what I used on #462:

bash -c 'source <(sed -n "/^SUITES=(/,/isolation)/p" test/run_all_versions.sh)
         echo "count=${#SUITES[@]}"; echo "last=${SUITES[-1]}"
         case " ${SUITES[*]} " in *" bloom_sizing "*) echo IN;; *) echo MISSING;; esac'

Expect count=126, last=isolation, bloom_sizing=IN after resolving.

This line has now conflicted four times today

#459 vs #462, #460 vs #462, #462 vs this, and #444 is queued behind it. That is not bad luck,
it is a structural property: every PR that adds a suite edits the same line, so any two of
them conflict by construction, and the failure mode is one that bash -n accepts and only
breaks at run time.

Worth fixing once the queue drains — one name per line, or generating the list from
test/*.sh with an explicit exclude for the non-suites. I have not filed it because I would
rather not add an issue that touches every open PR while four of them are in flight. Say the
word and I will, or take it yourself if you would rather.

No re-review needed from me after the rebase; my approval stands on the code, which I have
not asked you to change.