Batch the Gorilla bit writer: -13% more, now faster than TimescaleDB on load (#155)

[ChronicallyJD]

Batch the Gorilla bit writer and use a hardware bit-scan (#155)

Fourth and final lever on the #155 bulk-load cost. After #283/#284/#285 the profile's last large pgColumnar self-cost was encode_gorilla at ~10% (float4/float8 XOR encoding). Two bit-at-a-time loops drove it — the same pattern the other levers removed elsewhere:

1. bw_put() appended the field one bit per iteration. Now it merges bits into a 64-bit accumulator and flushes whole bytes at once (≤ 7 carried + ≤ 57 taken per chunk keeps it within 64 bits). Same LSB-first stream.
2. clz_in() / ctz_in() scanned for the leading/trailing set bit one step at a time. Now they call pg_leftmost_one_pos64() / pg_rightmost_one_pos64() (PostgreSQL's portable hardware bit-scan, port/pg_bitutils.h). Same counts.

Byte-identical

The encoded stream is unchanged by construction — bw_put writes the identical LSB-first bit sequence, just merged into bytes; the zero-counts are identical. Only the writer and its helpers changed; the reader (decode_gorilla / br_get) is untouched. Corroborated by an identical on-disk image over 100M rows and green roundtrip suites (a changed writer + unchanged reader passing roundtrip proves the new bytes decode identically).

Cleanroom

Standard bit-accumulator buffering + hardware bit-scan; implemented from the idea. The Gorilla scheme itself is Facebook's (Pelkonen et al., Gorilla, VLDB 2015) and was already present — this PR only speeds up the existing writer.

Measured (PG18, 100M-row TSBS-cpu; shared_buffers=1GB, work_mem=32MB)

Build	Load	vs baseline	On-disk bytes
baseline (main)	783.1 s	—	2,868,682,752
+ #283 (skip FSST build)	607.7 s	−22.4%	2,868,682,752
+ #284 (dict hash)	500.7 s	−36.1%	2,868,682,752
+ #285 (bitpack words)	439.1 s	−43.9%	2,868,682,752
+ this PR (Gorilla batch)	383.5 s	−51.0%	2,868,682,752

TimescaleDB compressed columnstore loads the same data in ~414 s — so pgColumnar is now at ~0.93× TS: faster than TimescaleDB on bulk load, from ~1.9× (twice as slow) at baseline. On-disk size is byte-identical across all five builds.

Correctness gate

gate_subset.sh on PostgreSQL 18.4 and 19beta2 (assert builds) — all suites green:

harness_selftest · native_writer · native_roundtrip · native_encoding
write_fsst_compressed · encode_effort · native_dml · native_skip · native_zonemap
GATE PASSED

Review order

Stacked on #285. Full chain: #283 → #284 → #285 → this. The diff here is Gorilla-only; please review or squash-merge them in sequence.

🤖 Generated with Claude Code

[jdatcmd]

Reviewed all four as a stack (identical SHAs, #286 contains everything). Local
five-major matrix on this tip: ALL VERSIONS PASSED. All four PRs show 10/10
checks, including the four aarch64 build jobs now that #279 has landed.

Splitting the review, because three of these are a different kind of change from
the fourth.

#284, #285, #286: rewrites that preserve output by construction

These re-express the same algorithm and emit the same bytes, so the risk is
implementation error rather than behaviour change. I checked the edges that
usually bite in bit-twiddling code and they are handled:

• bitpack emits byte at a time through explicit shifts, so the layout stays
  LSB-first and identical on any host endianness. width == 64 is special-cased
  out of the 1 << width UB, bitoff ? v >> (64 - bitoff) : 0 avoids the
  shift-by-64 on the other side, and the bound works out exactly:
  bytepos + nb <= ceil(totalbits/8) == nbytes, so the |= never runs off the
  buffer. Using |= rather than = is what makes the shared boundary byte
  correct.
• bw_put: <= 7 carried plus <= 57 taken keeps every chunk inside 64
  bits, and both masks are guarded against the 64-bit shift.
• clz_in / ctz_in use pg_leftmost_one_pos64 and
  pg_rightmost_one_pos64 from port/pg_bitutils.h rather than x86 intrinsics.
  That is the right choice and it matters more than it did last week: aarch64 is
  now built on every PR and runs the suites nightly, and _lzcnt_u64 would have
  failed there.
• The dictionary hash preserves first-seen assignment order, which is what
  makes the dictionary and the codes byte-identical to the linear build.

One nit, in the spirit of the thread we just had about comments outclaiming code:
the ctz_in comment says x == 0 "keeps the old all-zeros answer". The old loop
was while (((x >> n) & 1) == 0) n++;, which on x == 0 runs past 64 and is
undefined rather than returning 0. The new behaviour is strictly better; the
comment describes a prior behaviour that did not exist.

#283 is the one that changes a decision, and its claim is an argument

The other three cannot change stored bytes. This one can, and the PR says it does
not:

only skips when the dictionary is viable and wins for every vector, so the
stored bytes are identical

The chain is: distinct <= DICT_MAX_DISTINCT over the corpus, therefore the
dictionary is viable per vector, therefore the dictionary wins, therefore FSST is
never selected, therefore not building the table changes nothing.

The gate that actually decides whether FSST is attempted is
columnar_encoding.c:2330:

if (w == -1 && fsstTable != NULL && bestLen > rawLen - rawLen / 4)

so FSST is reached only when the best encoding so far saved less than 25%.
"The dictionary is viable" and "the dictionary saved at least 25%" are different
propositions, and the second is the one the gate tests. The case I could not
rule out by reading: a vector with close to 1024 distinct long values, where the
dictionary is viable but stores nearly every value plus codes, saves little, and
leaves bestLen above the threshold. That is a shape where FSST would have been
tried and might have won, and after this change the table is not there to try.

I could not demonstrate it. I swept distinct counts 200 to 3000 against short
and long shared-substring text and compared content hashes and sizes on both
branches: identical throughout. But FSST was not selected in any of those shapes
on main either, so the sweep failed to engage the mechanism and is weak
evidence rather than confirmation. I would rather say that than present it as a
pass.

What would settle it is the missing step in the argument, or a test. Either:
distinct <= 1024 implies the dictionary compresses by >= 25% for every 1024-row
vector (which I do not think holds for long values near the cap), or a fixture
that engages FSST on both sides of the cap and shows the bytes match.

Two smaller notes on the same function:

• The distinct probe scans corpus.len while the build it guards uses
  sampleLen. Conservative in the right direction, but worth a word since the
  two are deliberately different.
• FNV-1a collisions make the claim probabilistic rather than absolute. At <= 1024
  entries in 64 bits that is about 3e-14, so it is not a practical concern, but
  "byte-identical" is doing exact work in that sentence.
• 36 KB of stack for slot and used.

What none of the four has

No test pins any of this, and byte-identity is the crux of the whole stack. It is
cheap to assert: load a fixture, hash the stored bytes, and compare against the
same fixture loaded by the previous build. For #284, #285 and #286 the property is
"same bytes as before"; for #283 it is "same bytes as before, for corpora on both
sides of the cap", which is exactly the claim in question.

Performance

I have not independently reproduced the numbers, and I am not treating them as
verified. Given that a sequential sweep on the FSST margin work manufactured a
5-7% slowdown that reversed sign when interleaved, and understated a real win by
half, the question worth answering before these land as a performance claim is
whether these were interleaved or measured in sequence. Happy to run them on the
idle box the same way I did for #276 if you want an independent number.

Correctness and portability I am satisfied with. #283's byte-identity claim is the
one thing I would want closed before merge.

[jdatcmd]

Merged via #290, which carried your four commits unchanged plus the tests. GitHub
has marked all four of these merged, since the SHAs landed as they were: a66bee1,
e3d0217, d0f50a0, a570cc6.

Landing them together with the test was deliberate. #283 is the only one of the
four that changes a decision rather than rewriting a mechanism, and its
byte-identity claim had no evidence until the premise check existed. The other
three cannot change stored bytes, and the C self-test now proves that against a
reference implementation of the code each replaced rather than taking it on
reasoning.

Two things carried forward, neither blocking, both left for you:

• ctz_in's comment still says x == 0 "keeps the old all-zeros answer". The
  old loop was undefined on zero. Worth correcting, and worth knowing that the
  guard is load-bearing rather than defensive: with it removed,
  pg_rightmost_one_pos64 asserts and the backend dies, which is how the
  self-test's anti-vacuity control caught it.
• ColumnarFsstDictWins scans up to 16x the bytes the build samples (4 MB
  decide cap against a 256 KB sample cap). That direction is the safe one and
  should stay, but it deserves a sentence saying so, because the next reader will
  see two lengths and assume one is a typo.

One finding from the matrix worth more than this PR: utils/tuplestore.h arrives
transitively behind funcapi.h through PostgreSQL 18 and does not on 19. My
self-test built clean on 15 through 18 and failed on 19 alone. Same class as the
columnar_compat.h shims, and relevant to any future set-returning function.

Performance is still unmeasured from my side. That is next.