fix: do not store a validity bitmap for a chunk that holds no null (#1130)

CHANGELOG.md

@@ -18,6 +18,103 @@ true until the next version shipped.
 
 ### Fixed
 
+- The validity bitmap was stored uncompressed and never elided, so a column with
+  no nulls paid `ceil(rows / 8)` bytes for ever (#1130).
+
+  A column chunk's page was always `[validity bitmap][encoded values]`. The
+  bitmap is one bit per row and is written RAW, ahead of the block codec, which
+  therefore never saw it. A `NOT NULL` column, or one that simply holds no nulls,
+  still stored `ceil(rows / 8)` bytes of `0xFF`.
+
+  The writer now omits the bitmap for a chunk that holds no null, and says so in
+  the encoding descriptor's new flags byte. Measured on ClickBench
+  `hits_0.parquet` (1,000,000 rows, 105 columns, no null in any of them),
+  imported through `pgcolumnar.import_parquet` on PG17, both arms loaded into the
+  same cluster on the same day:
+
+      stored chunk bytes   78,109,810 -> 64,984,810   -16.80%
+      validity bytes       13,125,000 ->          0
+      relation size        78,381,056 -> 65,216,512
+      chunks without a bitmap    0 of 735 -> 735 of 735
+
+  The saving is exactly the bitmap: -13,125,000 bytes, which is
+  105 columns x 125,000 bytes. Nothing else moved.
+
+  **DECIDED FROM THE ROWS WRITTEN, not from the column's `NOT NULL` flag.** A
+  nullable column whose rows happen to be complete gets the saving too, and a
+  constraint added later cannot make an already-written chunk lie.
+
+  **THE FORMAT MOVES: encoding descriptor version 2 -> 3.** Version 3 spends the
+  byte version 2 wrote as a zero reserved byte, so every field keeps its offset
+  and a version-2 descriptor is a version-3 one whose flags are clear. Readers
+  accept 2 and 3; writers emit 3. Tables written by an older build keep reading
+  with no conversion.
+
+  **DOWNGRADING A BINARY BELOW THE ONE THAT WROTE THE TABLE IS NOT SUPPORTED**,
+  and the shape of that failure is measured rather than asserted. An alpha4
+  binary reading a table this build wrote: a sequential scan always raises
+  `unrecognized native encoding descriptor`; an index fetch of a chunk narrower
+  than the bitmap raises `validity bitmap longer than the chunk`; and an index
+  fetch of a WIDER chunk returned NULL for a row that holds a value in 16 of 40
+  single-row fetches, because the old reader tests a bit in bytes that are not a
+  bitmap before it reaches any version check. #1137 tracks the versioning gap
+  that makes the silent case possible.
+
+  **THE BITMAP'S SIZE IS NOW A PROPERTY OF THE CHUNK, NOT THE ROW GROUP**, and
+  three readers had to learn that, not one. The first implementation taught only
+  the sequential scan, and the PG17 matrix went red in 33 suites --
+  `native_index`'s point lookup returned NO ROW for a row that is there. The two
+  are correlated rather than independent: `pgcolumnar_fetch_coalesce_read` skips
+  any chunk whose `page_length` is below the group's bitmap size, and eliding the
+  bitmap is exactly what takes a well-encoded chunk below it, so the chunks this
+  change helps most are the ones that fall to the per-column fetch path.
+
+  **A HARDENING GAP THE CHANGE'S OWN COMMENTS FORCED INTO THE OPEN.** The scan's
+  fast path for fixed-width by-value types read a value without checking it
+  against the end of the stream, trusting the validity bitmap to stop first.
+  A synthesized all-ones bitmap is a new way to reach past it, so the fast path
+  now carries the bound the general path has always had. Two more guards refuse
+  a descriptor that claims no bitmap while accounting for fewer values than the
+  group has rows, and a row count whose bitmap would not fit in memory. Both are
+  asserted by SQLSTATE `XX001` against a poisoned catalog.
+
+  THE BOUND ON THE FAST PATH COSTS NOTHING MEASURABLE. Backend instructions for
+  `sum` over a 1,000,000-row bigint column, `cpu_core/instructions/` pinned to
+  the P-cores, three repetitions per backend and two backends per arm:
+
+      with the bound      6,819,185,307   6,811,086,223
+      without it          7,408,953,743   7,394,084,968
+
+  The bounded build is 7.9% LOWER, reproduced in a second build directory with
+  the arms interleaved (6,818,778,149 / 6,801,164,841 against the same .so).
+  WHAT THIS SUPPORTS IS THAT THE CHECK IS NOT A COST. It is not claimed as a
+  saving, and the direction is unexplained: the compiled function is LARGER with
+  the bound (97 instructions against 88, plus 69 bytes of cold block) and nothing
+  else in the object moved, so a local codegen effect cannot produce a gap of
+  197 instructions per row.
+
+  THE PLAN AND THE WORK ARE THE SAME ON BOTH ARMS, which was the first thing to
+  suspect and is now excluded. Captured per arm: the same plan node
+  (`Custom Scan (PgColumnarScan)`), the same `actual rows=1000000`, the same
+  7 chunk groups and 100 vectors decoded, and the same checksum over the column.
+  So neither the plan nor the fixture explains it.
+
+  WHAT DOES NARROW IT IS THE BRANCH COUNT. Measured beside the instructions:
+
+      branches         1,679,418,138   against   1,675,963,100      +0.2%
+      instructions     6,817,514,459   against   7,409,086,680      +8.7%
+
+  Equal control flow, 590 million more instructions retired. So the difference
+  is the instruction MIX in straight-line code and not more iterations of
+  anything. The mechanism is still unidentified, and the number is recorded as
+  what it is: the check is not a cost, and nothing further is claimed.
+
+  `test/validity_elision.sh` and `test/pytest/test_validity_elision.py`, 25
+  checks each, green on PG15 through PG19. Three mutations were run against
+  them: forcing the writer's elision decision false reddens one arm, making the
+  reader ignore the flag reddens ten, and using the group-wide size in the fetch
+  path reddens exactly the three fetch arms. Ledger rows seeded from five real
+  runs merged in one call, so each carries 15;16;17;18;19.
 - Three suites ported to pytest, and the queue re-derived (#432).
 
   `analyze_reltuples`, `projection_update` and `projection_drop_column`, 21 names,

design/CASCADE_ENCODING_PLAN.md

@@ -162,7 +162,7 @@ of incompressible columns would spend less time in candidates that bail early.
 Cascading (step 2) is unaffected by this result. It remains the size lever, and
 it remains a format change, though a smaller one than the plan assumed: the
 encoding descriptor is already versioned (`COLUMNAR_NATIVE_ENCDESC_VERSION`, now
-2) with a fixed-size entry per vector, and `columnar_reader.c` rejects an
+3, since #1130 spent the header's reserved byte on flags) with a fixed-size entry per vector, and `columnar_reader.c` rejects an
 unrecognized version with a clean error. A version 3 entry carrying a chain can
 therefore coexist with 2, and an older build meets a clear error rather than a
 wrong value. The open decision is still whether new tables write version 3 by

design/CASCADE_FORMAT_SPEC.md

@@ -22,21 +22,38 @@ of them names a *component* of a scheme's output, not the whole of it.
 The per-chunk `encoding_descriptor` in `pgcolumnar.column_chunk` is:
 
 ```
-[uint8  version]        COLUMNAR_NATIVE_ENCDESC_VERSION = 2
-[uint8  reserved]
+[uint8  version]        COLUMNAR_NATIVE_ENCDESC_VERSION = 3
+[uint8  flags]          bit 0 NO_VALIDITY: this chunk stored no validity bitmap
 [uint32 vectorCount]
 [vectorCount entries of 13 bytes]
     [uint8  encodingType]
+    [uint32 valueCount]
     [uint32 rawLen]
     [uint32 encLen]
-    [uint32 valueCount]
 [uint32 sharedTableLen]     optional, FSST's chunk-shared symbol table
 [sharedTableLen bytes]
 ```
 
-Verified: the descriptor is versioned, and `columnar_reader.c` rejects an
-unrecognized version with `ERRCODE_DATA_CORRUPTED` before reading anything else,
-so version 3 chunks can coexist with version 2 in one table.
+The entry's field ORDER above is `valueCount, rawLen, encLen`, which is what
+`columnar_encdesc.h` writes and reads (offsets 1, 5, 9 within the entry). This
+block previously named them in the wrong order; no code ever followed it.
+
+Version 3 (#1130) spends the byte version 2 wrote as a zero reserved byte. A
+version-2 descriptor is therefore a version-3 one whose flags are all clear:
+every field keeps its offset, and a reader needs no second parse. Writers emit
+`COLUMNAR_NATIVE_ENCDESC_VERSION`; readers accept anything from
+`COLUMNAR_NATIVE_ENCDESC_MIN_READABLE` (2) upward, because tables written by an
+older build must keep reading.
+
+`NO_VALIDITY` says the chunk held no null and its validity bitmap was therefore
+not written: the page is `[encoded]` rather than `[validity][encoded]`. It is a
+property of ONE CHUNK -- one column can hold nulls while its neighbour does not
+-- so every reader decides the bitmap's size per chunk rather than once per row
+group from `ceil(rowCount / 8)`.
+
+Verified: the descriptor is versioned, and `columnar_reader.c` rejects a version
+it does not recognize with `ERRCODE_DATA_CORRUPTED` before reading anything
+else, so version 2 and version 3 chunks coexist in one table.
 
 ## Decision 1: which feature is being built
 
@@ -64,7 +81,23 @@ If the answer is whole-output chaining after all, this spec's descriptor is clos
 to right and the candidate list needs rewriting. If it is component cascading, the
 descriptor below is the one to implement.
 
-## Decision 2: when a table starts writing version 3
+## THE VERSION NUMBER THIS DOCUMENT PROPOSES IS TAKEN (2026-09-18)
+
+This spec was written when the next descriptor version was free. It is not:
+**#1130 spent version 3** on the header's flags byte, so that a chunk holding no
+null can say it stored no validity bitmap -- 16.8% of a real ClickBench table.
+That change keeps every existing field at its offset and adds no per-vector
+bytes, so it is compatible in shape with everything below; only the number
+moves.
+
+**Read every "version 3" below as "the next version", which is now 4.** The two
+decisions the sections below record -- when a table starts writing it, and what
+the entry looks like -- are unaffected. Decision 2's recommendation is now
+partly settled by precedent rather than by argument: #1130 took the
+unconditional break, so a second one costs an operator nothing new if it ships
+in the same release, and rather more if it ships later.
+
+## Decision 2: when a table starts writing the new version
 
 - **Unconditionally, once the feature ships.** The break is deterministic and tied
   to the upgrade, so it is one CHANGELOG line: chunks written after this version
@@ -85,7 +118,7 @@ An earlier draft of this spec recommended the data-triggered variant. That was
 wrong for the reason above: it trades a predictable break for an unpredictable
 one, which is worse to operate even though it breaks fewer tables.
 
-## Proposed version 3 entry (component cascading, one component per stage)
+## Proposed entry for the NEXT version, 4 (component cascading, one component per stage)
 
 ```
 [uint8  chainLen]          1..PGCN_MAX_CHAIN (proposed 3)
@@ -110,8 +143,9 @@ Without them the reader is guessing. The alternative, making every encoding's
 output self-describing, changes every encoder's on-disk bytes and is a much larger
 change than this one.
 
-`chainLen == 1` expresses exactly what version 2 expresses, so version 3 is a
-superset.
+`chainLen == 1` expresses exactly what version 2 expresses, so the new version is
+a superset -- of version 3 as well, whose flags byte sits in the header and is
+untouched by anything here.
 
 ## Guards, each needing a test that fails without it
 

design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md

@@ -84,6 +84,14 @@ page checksums apply, as in the 1.0-dev line.
   vector granularity).
 - Null values are recorded per column chunk as a validity bitmap, one bit per row,
   laid out so a vector's validity slice is a contiguous run.
+- **A column chunk that holds no null stores no bitmap at all**, and says so in its
+  encoding descriptor's flags byte (`NO_VALIDITY`, descriptor version 3). The
+  bitmap is written ahead of the block codec and is never compressed, so on a
+  column with no nulls it was a constant cost that grew with the row count and
+  shrank with nothing: measured at 16.8% of a 1,000,000-row ClickBench table, and
+  99.5% of the page on a well-encoded column. Elision is decided per CHUNK from
+  the rows actually written, not from the column's `NOT NULL` constraint, so a
+  nullable column whose rows happen to be complete gets it too.
 
 ## 5. Encodings
 

docs/ARCHITECTURE.md

@@ -15,8 +15,10 @@ A columnar relation stores its data in its own main fork using standard
 PostgreSQL pages, so the buffer manager, WAL, and page checksums apply. Block 0
 is a metapage, block 1 is reserved, and block 2 onward is a logical byte area.
 Rows are grouped into row groups (a run of up to `stripe_row_limit` rows, the
-write unit). Within a row group one column's data is a chunk, holding a validity
-bitmap and a value stream encoded in fixed-size vectors. The metadata catalog is a separate set of ordinary heap tables in the
+write unit). Within a row group one column's data is a chunk. A chunk holds a
+value stream encoded in fixed-size vectors. A validity bitmap precedes that
+stream when the chunk holds a null. A chunk with no nulls stores no bitmap, and
+records that in its encoding descriptor. The metadata catalog is a separate set of ordinary heap tables in the
 `pgcolumnar` schema:
 
 - `storage`, `row_group` and `column_chunk` record the layout.

docs/limitations.md

@@ -67,7 +67,26 @@ columnar format version` for the metapage, or `unsupported columnar native forma
 version` for the data format. The read then fails. It does not read bytes that a
 different layout wrote. The check runs on each decode path. These paths include
 the sequential scan, the vectorized aggregate, and the index-scan fetch. Thus the
-build refuses a version that it cannot read, whichever path the query uses. Both guards are pinned by
+build refuses a version that it cannot read, whichever path the query uses.
+
+**A third version is not covered by either guard.** Each column chunk carries an
+encoding descriptor with its own version. The two stamps above do not move when
+it does. A build reads descriptor versions 2 and 3. It refuses a higher one with
+`unrecognized native encoding descriptor`.
+
+That refusal is late rather than early. It fires when a chunk is decoded, not
+when the relation is opened. The index-scan fetch path consults a chunk for
+null-ness before it decodes it. A descriptor from a future build can therefore
+produce a NULL where a value exists. Measured on an alpha4 build reading alpha5
+chunks: 16 of 40 single-row fetches returned NULL. The other 24 raised the
+error.
+
+**Do not downgrade the extension below the version that wrote a table.** It is
+not merely inadvisable. One path can return a wrong answer rather than fail.
+Upgrading is safe and needs no conversion. A newer build reads every descriptor
+version it has ever written. [Issue
+#1137](https://github.com/commandprompt/pgcolumnar/issues/1137) tracks closing
+the gap, so that a future version is refused early on every path. Both guards are pinned by
 `test/native_format.sh`.
 
 A projection stores its own copy of the data and carries its own format version.

docs/user-guide.md

@@ -326,7 +326,7 @@ bookkeeping and none of the data:
 
 ```
 table pgcolumnar.row_group:    INSERT: storage_id[bigint]:10000000000 group_number[bigint]:1 ...
-table pgcolumnar.column_chunk: INSERT: ... encoding_descriptor[bytea]:'\x020001...'
+table pgcolumnar.column_chunk: INSERT: ... encoding_descriptor[bytea]:'\x030100...'
 table pgcolumnar.zone_map:     INSERT: ... minimum[bytea]:'\x0100000000000000' ...
 table pgcolumnar.delete_vector: UPDATE: ... bitmap[bytea]:'\x03' deleted_count[integer]:2
 ```

src/columnar.h

@@ -52,9 +52,10 @@
  * single 0 byte: raw present values, no per-vector encoding, block_codec 0) or a
  * D4 descriptor with this leading version byte, recording the lightweight
  * encoding chosen per 1024-value vector so the reader reconstructs the exact raw
- * value stream. The layout is: uint8 version, uint8 reserved, uint32 vectorCount,
+ * value stream. The layout is: uint8 version, uint8 flags, uint32 vectorCount,
  * then per vector { uint8 encodingType, uint32 valueCount, uint32 rawLen,
  * uint32 encLen }. Integers are host-endian (little-endian hosts assumed, spec 3).
+ * Byte 1 was a reserved zero until version 3 spent it on flags.
  *
  * Version 2 (E3b) appends one trailing region after the per-vector entries:
  * { uint32 sharedTableLen, sharedTableLen bytes }. It holds a chunk-shared FSST
@@ -63,9 +64,60 @@
  * their own, so the costly table build is paid once per chunk, not per vector.
  * It is appended (not inserted after the header) so every per-vector entry offset
  * is unchanged from version 1.
+ *
+ * Version 3 (#1130) spends the header's reserved byte on flags, whose only bit
+ * today says the chunk stored no validity bitmap because it holds no null. No
+ * field moves, nothing is added per vector, and a version-2 descriptor is a
+ * version-3 one whose flags are clear.
  */
 #define COLUMNAR_NATIVE_ENCDESC_BASELINE 0
-#define COLUMNAR_NATIVE_ENCDESC_VERSION 2
+#define COLUMNAR_NATIVE_ENCDESC_VERSION 3
+
+/*
+ * The oldest descriptor a reader accepts. Version 3 (#1130) spends the header's
+ * previously-reserved byte on flags, so a version-2 descriptor is a version-3
+ * one whose flags are all clear -- every field keeps its offset and the reader
+ * needs no second parse. Writers emit VERSION; readers accept anything from
+ * MIN_READABLE to VERSION, because tables written by an older build must keep
+ * reading.
+ *
+ * AN OLD BINARY READING A VERSION-3 TABLE IS MOSTLY LOUD AND NOT ALWAYS, and the
+ * difference is measured rather than argued. Run on 2026-09-18 against an alpha4
+ * build (descriptor v2) reading tables this code wrote:
+ *
+ *   sequential scan      ERROR "unrecognized native encoding descriptor", every
+ *                        time, from the guard in pgcolumnar_native_decode_chunk.
+ *   index fetch, narrow  ERROR "columnar chunk for column N has a validity bitmap
+ *     chunk              longer than the chunk" -- the elided chunk is smaller
+ *                        than the bitmap the old reader expects, so its own
+ *                        bound refuses it.
+ *   index fetch, wide    40 single-row fetches: 24 raised "unrecognized native
+ *     chunk              encoding descriptor" and 16 RETURNED NULL for a row that
+ *                        holds a value. The old reader takes the chunk's first
+ *                        ceil(rowCount / 8) encoded bytes for a bitmap and tests
+ *                        one bit; a zero bit answers "null" before any version
+ *                        check is reached.
+ *
+ * So downgrading a binary below the one that wrote the table is NOT supported,
+ * and this is the shape of the failure rather than a guarantee about it.
+ * pgcolumnar.storage.format_version is the stamp that could refuse such a table
+ * early on both paths, and it cannot express this change today: it is checked
+ * `!= COLUMNAR_NATIVE_VERSION_MAJOR`, so bumping it would also make THIS build
+ * refuse every table alpha4 wrote. Tracked separately; see the issue linked from
+ * the #1130 changelog entry.
+ */
+#define COLUMNAR_NATIVE_ENCDESC_MIN_READABLE 2
+
+/*
+ * Header flags, at byte 1, which version 2 wrote as a zero reserved byte.
+ *
+ * NO_VALIDITY says the column chunk holds no nulls and its validity bitmap was
+ * therefore NOT written: the page is [encoded] rather than [validity][encoded].
+ * The bitmap is one bit per row, written raw ahead of the block codec, which
+ * never sees it -- measured at 16.80% of a 1,000,000-row ClickBench table where
+ * no column has a null, and 99.5% of the page on a column that encodes well.
+ */
+#define COLUMNAR_ENCDESC_FLAG_NO_VALIDITY 0x01
 #define COLUMNAR_NATIVE_ENCDESC_HEADER_LEN 6	/* version + reserved + vectorCount */
 #define COLUMNAR_NATIVE_ENCDESC_ENTRY_LEN 13	/* encodingType + 3 * uint32 */
 #define COLUMNAR_NATIVE_ENCDESC_SHARED_LEN_BYTES 4	/* trailing uint32 sharedTableLen */