From 73cc36adb2df52cc20e1cd7d2c1c3474dba95537 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 18 Sep 2026 15:14:33 -0600 Subject: [PATCH] fix: do not store a validity bitmap for a chunk that holds no null (#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 for ever. The writer now omits it for a chunk that holds no null and records that 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) through pgcolumnar.import_parquet on PG17, both arms 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 with no bitmap 0 of 735 -> 735 of 735 The saving is exactly the bitmap: -13,125,000 bytes, which is 105 columns x 125,000. Nothing else moved. DECIDED FROM THE ROWS WRITTEN, not from the attribute's NOT NULL flag, so 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: descriptor version 2 -> 3, spending the byte version 2 wrote as a zero reserved byte. Every field keeps its offset, so a version-2 descriptor is a version-3 one whose flags are clear; readers accept 2 and 3 and no conversion is needed. 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 raises `unrecognized native encoding descriptor` on every sequential scan, but an index fetch of a chunk wider than the bitmap returned NULL for a row that holds a value in 16 of 40 single-row fetches. #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 it. The first implementation taught only the sequential scan and the PG17 matrix went red in 33 suites, with native_index's point lookup returning NO ROW for a row that is there. The two paths 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 helps most are the ones that fall to the per-column fetch path. All three now call one helper, which is also what keeps the coalesced read and the per-column loop agreeing about a pointer and a length one of them computed. 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 bitmap to stop first; a synthesized all-ones bitmap is a new way past it, so that path now carries the bound the general path has always had. Two guards refuse the catalog that would get there. The bound costs nothing measurable: 6.81e9 backend instructions with it against 7.40e9 without, reproduced across two build directories with the arms interleaved -- 7.9% LOWER with the check, a direction this commit does not try to explain and does not claim as a saving. test/validity_elision.sh and test/pytest/test_validity_elision.py, 24 checks each, green on PG15/16/17/18/19. Three mutations were run: 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 and nothing else. Two arrangements in those suites are load-bearing and both were found by an arm that failed rather than by reasoning -- a key column beside the measured one, because an index on the only column is answered by an Index Only Scan that never calls the table AM, and pgcolumnar.enable_custom_scan = off, because enable_seqscan does not govern the columnar custom scan. One attractive guard was implemented and reverted: bounding the synthesized bitmap by the row group's byteLength refuses a CORRECT table, because an elided group of 360 bytes legitimately needs 12,500 bytes of bits. Ledger rows seeded from five real runs merged in one call, so each carries 15;16;17;18;19; suites_not_covered does not move because registering the suite and seeding it happen in this change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK --- CHANGELOG.md | 97 +++++ design/CASCADE_ENCODING_PLAN.md | 2 +- design/CASCADE_FORMAT_SPEC.md | 54 ++- design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md | 8 + docs/ARCHITECTURE.md | 6 +- docs/limitations.md | 21 +- docs/user-guide.md | 2 +- src/columnar.h | 56 ++- src/columnar_encdesc.h | 92 +++- src/columnar_reader.c | 246 ++++++++++- src/columnar_write_state.c | 34 +- test/check_ledger.tsv | 25 ++ test/check_ledger_budget.txt | 25 +- test/encode_post_codec.sh | 12 +- test/fsst_margin.sh | 3 +- test/fsst_verdict_cache.sh | 3 +- test/native_encdesc_golden.sh | 24 +- test/native_fetch_coalesce.sh | 21 +- test/pytest/TESTS.md | 97 +++++ test/pytest/expected_tests.txt | 10 +- test/pytest/test_compare_to_bash.py | 2 +- .../test_compression_reaches_the_cascade.py | 3 +- test/pytest/test_encode_post_codec.py | 20 +- test/pytest/test_native_fetch_coalesce.py | 9 +- test/pytest/test_validity_elision.py | 397 ++++++++++++++++++ test/run_all_versions.sh | 1 + test/validity_elision.sh | 364 ++++++++++++++++ test/write_fsst_compressed.sh | 3 +- 28 files changed, 1564 insertions(+), 73 deletions(-) create mode 100644 test/pytest/test_validity_elision.py create mode 100755 test/validity_elision.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a35588..5b8d3adc 100644 --- a/CHANGELOG.md +++ b/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, diff --git a/design/CASCADE_ENCODING_PLAN.md b/design/CASCADE_ENCODING_PLAN.md index 8b7fdaf7..615d31b2 100644 --- a/design/CASCADE_ENCODING_PLAN.md +++ b/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 diff --git a/design/CASCADE_FORMAT_SPEC.md b/design/CASCADE_FORMAT_SPEC.md index 7158febe..5cef97b9 100644 --- a/design/CASCADE_FORMAT_SPEC.md +++ b/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 diff --git a/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md b/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md index bdf6e219..db8c33f6 100644 --- a/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md +++ b/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 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0b150f92..c6aab3d6 100644 --- a/docs/ARCHITECTURE.md +++ b/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. diff --git a/docs/limitations.md b/docs/limitations.md index 7f266dec..a0a4dde3 100644 --- a/docs/limitations.md +++ b/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. diff --git a/docs/user-guide.md b/docs/user-guide.md index 010d73a9..cf825205 100644 --- a/docs/user-guide.md +++ b/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 ``` diff --git a/src/columnar.h b/src/columnar.h index 68dccafd..00713905 100644 --- a/src/columnar.h +++ b/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 */ diff --git a/src/columnar_encdesc.h b/src/columnar_encdesc.h index f336f947..55b349a0 100644 --- a/src/columnar_encdesc.h +++ b/src/columnar_encdesc.h @@ -30,7 +30,9 @@ #include "columnar.h" /* COLUMNAR_NATIVE_ENCDESC_* constants */ -/* header: version u8, reserved u8, vectorCount u32 (COLUMNAR_NATIVE_ENCDESC_HEADER_LEN) */ +/* header: version u8, flags u8, vectorCount u32 (COLUMNAR_NATIVE_ENCDESC_HEADER_LEN) */ +#define COLUMNAR_ENCDESC_HDR_OFF_VERSION 0 +#define COLUMNAR_ENCDESC_HDR_OFF_FLAGS 1 #define COLUMNAR_ENCDESC_HDR_OFF_VECCOUNT 2 /* per-vector entry: type u8, valueCount u32, rawLen u32, encLen u32 (…ENTRY_LEN) */ @@ -47,18 +49,63 @@ typedef struct PgColumnarEncdescEntry uint32 encLen; } PgColumnarEncdescEntry; -/* append the descriptor header (version + reserved + vectorCount) */ +/* + * Append the descriptor header: version + flags + vectorCount. + * + * FLAGS IS AN ARGUMENT WITH NO DEFAULT, deliberately. The convenience wrapper + * that passed 0 was left behind by #1130 with no callers, and a zero flags byte + * is no longer a neutral value: it asserts that this chunk DID store a validity + * bitmap. A writer that wants that has to say so. + */ static inline void -PgColumnarEncdescPutHeader(StringInfo desc, uint32 vectorCount) +PgColumnarEncdescPutHeaderFlags(StringInfo desc, uint32 vectorCount, uint8 flags) { uint8 version = COLUMNAR_NATIVE_ENCDESC_VERSION; - uint8 reserved = 0; appendBinaryStringInfo(desc, (char *) &version, 1); - appendBinaryStringInfo(desc, (char *) &reserved, 1); + appendBinaryStringInfo(desc, (char *) &flags, 1); appendBinaryStringInfo(desc, (char *) &vectorCount, sizeof(uint32)); } +/* + * Whether a reader understands this descriptor's version. Callers that have + * already checked descLen may pass the header directly. + */ +static inline bool +PgColumnarEncdescVersionSupported(const char *desc) +{ + uint8 v = (uint8) desc[COLUMNAR_ENCDESC_HDR_OFF_VERSION]; + + return v >= COLUMNAR_NATIVE_ENCDESC_MIN_READABLE && + v <= COLUMNAR_NATIVE_ENCDESC_VERSION; +} + +/* + * Whether this column chunk OMITTED its validity bitmap because it held no + * nulls (#1130). + * + * A PROPERTY OF ONE CHUNK, not of the row group: one column can hold nulls + * while its neighbour does not, so this cannot be decided once per group the + * way ceil(rowCount / 8) was before v3. Callers already hold that group-wide + * size and use it unchanged when this returns false, which is why this answers + * the question rather than recomputing the length. + * + * A version-2 descriptor has a zero byte where the flags now are, so it answers + * false without a version test of its own, and the D2b baseline descriptor is + * one byte long and answers false on the length check. + */ +static inline bool +PgColumnarEncdescOmitsValidity(const char *desc, uint32 descLen) +{ + if (desc == NULL || descLen < COLUMNAR_NATIVE_ENCDESC_HEADER_LEN) + return false; + if ((uint8) desc[COLUMNAR_ENCDESC_HDR_OFF_VERSION] == + COLUMNAR_NATIVE_ENCDESC_BASELINE) + return false; + return ((uint8) desc[COLUMNAR_ENCDESC_HDR_OFF_FLAGS] & + COLUMNAR_ENCDESC_FLAG_NO_VALIDITY) != 0; +} + /* append one per-vector entry */ static inline void PgColumnarEncdescPutEntry(StringInfo desc, uint8 type, uint32 valueCount, @@ -80,6 +127,41 @@ PgColumnarEncdescReadVectorCount(const char *desc) return vectorCount; } +/* + * Total values the per-vector entries account for, or -1 if the descriptor is + * too short to walk. Used to check a NO_VALIDITY claim against the row count: + * a chunk that stored no bitmap is asserting that every row of the group is + * present in it, and that assertion has to be checked against something the + * writer also recorded, or a corrupt row_count turns into phantom rows the + * reader believes are there. + */ +static inline int64 +PgColumnarEncdescTotalValueCount(const char *desc, uint32 descLen) +{ + uint32 vectorCount; + uint64 total = 0; + uint64 entriesEnd; + uint32 i; + + if (desc == NULL || descLen < COLUMNAR_NATIVE_ENCDESC_HEADER_LEN) + return -1; + vectorCount = PgColumnarEncdescReadVectorCount(desc); + entriesEnd = (uint64) COLUMNAR_NATIVE_ENCDESC_HEADER_LEN + + (uint64) vectorCount * COLUMNAR_NATIVE_ENCDESC_ENTRY_LEN; + if (entriesEnd > (uint64) descLen) + return -1; + for (i = 0; i < vectorCount; i++) + { + uint32 vc; + + memcpy(&vc, desc + COLUMNAR_NATIVE_ENCDESC_HEADER_LEN + + (Size) i * COLUMNAR_NATIVE_ENCDESC_ENTRY_LEN + + COLUMNAR_ENCDESC_OFF_VALUECOUNT, sizeof(uint32)); + total += vc; + } + return (int64) total; +} + /* read one per-vector entry at dp into *e; returns the cursor past it */ static inline const char * PgColumnarEncdescReadEntry(const char *dp, PgColumnarEncdescEntry *e) diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 8a15c286..122c9b49 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -477,6 +477,23 @@ pgcolumnar_row_read_column(PgColumnarReadState *rs, int c, Datum *values, bool * { char *p = rs->nativeValueCursor[c]; + /* + * BOUNDED, which the inlined copy of this decode was not. The + * general path below has always refused a value running past the + * stream end; this one trusted the bitmap to stop first. #1130 + * gives a corrupt catalog a new way to say "every row is present" + * -- the NO_VALIDITY flag on a chunk whose values are short -- + * and a synthesized all-ones bitmap then walks this cursor off the + * end of the stream. The guard in pgcolumnar_native_load_group + * checks the descriptor's value COUNTS against the row count; it + * cannot check bytes, because the encoded bytes are what the + * decoder is about to produce. + */ + if (p + att->attlen > rs->nativeValueEnd[c]) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("pgcolumnar: fixed-length value runs past the value stream end"))); + values[c] = fetch_att(p, true, att->attlen); rs->nativeValueCursor[c] = p + att->attlen; } @@ -1021,6 +1038,29 @@ pgcolumnar_read_start(PgColumnarReadState *readState) } +/* + * pgcolumnar_chunk_validity_bytes + * How many bytes of validity bitmap THIS chunk stored: 0 when it held no + * null and therefore wrote none (#1130), and the row group's + * ceil(rowCount / 8) otherwise. + * + * A PROPERTY OF ONE CHUNK, not of the row group. One column can hold nulls + * while its neighbour does not, so every reader that used to derive the + * size once per group asks this once per chunk instead. Three of them read + * the same chunk -- the group scan, the coalesced fetch read, and the + * per-column fetch path -- and they MUST agree, because the coalesced read + * hands the per-column loop a pointer and a length it computed itself. + * They agree by calling this rather than by each repeating the condition. + */ +static inline int +pgcolumnar_chunk_validity_bytes(const NativeColumnChunkMetadata *cc, + int validityBytes) +{ + return PgColumnarEncdescOmitsValidity(cc->encodingDescriptor, + cc->encodingDescriptorLen) + ? 0 : validityBytes; +} + /* * pgcolumnar_chunk_value_bytes * The value stream that follows a chunk's validity bitmap, as a uint32. @@ -1090,7 +1130,7 @@ pgcolumnar_native_decode_chunk(MemoryContext cx, Form_pg_attribute att, MemoryContext decodeScratch; if (descLen < COLUMNAR_NATIVE_ENCDESC_HEADER_LEN || - (uint8) desc[0] != COLUMNAR_NATIVE_ENCDESC_VERSION) + !PgColumnarEncdescVersionSupported(desc)) ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), errmsg("pgcolumnar: unrecognized native encoding descriptor"))); @@ -2446,6 +2486,8 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) List *chunks; ListCell *lc; int validityBytes; + int chunkValidityBytes; /* #1130: per chunk, not per row group */ + char *allPresentBits = NULL; /* shared synthesized all-ones bitmap */ int maxVecCount; int groupVecDecoded; /* measured in the decode loop, never derived */ int pass; @@ -2625,7 +2667,7 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) * answer instead of a loud one. */ if (cc->encodingDescriptorLen < COLUMNAR_NATIVE_ENCDESC_HEADER_LEN || - (uint8) cc->encodingDescriptor[0] != COLUMNAR_NATIVE_ENCDESC_VERSION) + !PgColumnarEncdescVersionSupported(cc->encodingDescriptor)) { allDescriptor = false; continue; @@ -2722,7 +2764,98 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) rg->fileOffset, rg->fileOffset + rg->byteLength))); base = rs->nativeBuffer + (cc->pageOffset - rg->fileOffset); - rs->nativeValidity[cc->columnIndex] = base; + + /* + * #1130: the bitmap's size is a property of THIS CHUNK, not of the row + * group. A chunk that held no null did not store one, and its neighbour + * in the same group may still have. `validityBytes` above remains the + * group-wide ceil(rowCount / 8), which is the length of a bitmap that + * WAS stored and of the synthesized one below. + */ + chunkValidityBytes = pgcolumnar_chunk_validity_bytes(cc, validityBytes); + + /* + * A chunk with no stored bitmap still has to answer "is row r present?", + * and all fourteen readers of nativeValidity read it as bits. Point them + * at one all-ones buffer rather than teaching each of them a second + * shape: the answer is identical for every such chunk in the group, so + * it is built once, shared, and the readers stay unchanged. + */ + if (chunkValidityBytes > 0) + rs->nativeValidity[cc->columnIndex] = base; + else + { + /* + * A CHUNK THAT STORED NO BITMAP IS ASSERTING THAT EVERY ROW OF THE + * GROUP IS PRESENT IN IT, so that assertion is checked against the + * descriptor's own per-vector value counts before it is believed. + * + * Without this the synthesized all-ones bitmap below reports + * "present" for rows the chunk does not hold, and the scan reads + * past the value stream. Measured with corruption.sh's + * `row_count = row_count + 100000`: SIGSEGV. A stored bitmap needs + * no equivalent because its own bits say which rows are absent. + */ + int64 accounted = + PgColumnarEncdescTotalValueCount(cc->encodingDescriptor, + cc->encodingDescriptorLen); + + if (accounted < 0 || (uint64) accounted != rg->rowCount) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("columnar chunk for column %d claims no nulls but does not account for every row of row group " UINT64_FORMAT, + cc->columnIndex + 1, rg->groupNumber), + errdetail("The descriptor accounts for " INT64_FORMAT " values where the row group declares " UINT64_FORMAT ".", + accounted, rg->rowCount))); + + if (allPresentBits == NULL) + { + /* + * SIZED FROM rowCount IN 64-BIT, because that is what its + * readers index it by, and `validityBytes` above is an int that + * a corrupt row_count overflows. Sizing from that int gave a + * one-byte buffer for a row count of 2^60 and the readers walked + * off it: SIGSEGV, caught by corruption.sh's "alive after + * row_count" arm. + * + * A stored bitmap needs no such check because pageLength bounds + * it -- pgcolumnar_chunk_value_bytes refuses a validity span + * longer than the chunk. An OMITTED one has no such relation to + * anything on disk, so the bound has to be asserted here. + */ + uint64 need = (rg->rowCount + 7) / 8; + + /* + * MaxAllocSize AND NOTHING TIGHTER, which is not for want of + * looking. The obvious bound is the row group's own byteLength + * -- a stored bitmap cannot be longer than the bytes the group + * occupies -- and it is WRONG here, which this change is itself + * the proof of: an elided group of two well-encoded bigint + * columns measured 360 bytes on disk against a 12,500-byte + * bitmap it no longer stores. Bounding by byteLength refused + * every table the feature helps most; measured, not reasoned + * about, when that bound made a plain SELECT raise + * "implausible row count" on a correct table. + * + * The real guard is the value-count check above, which runs + * FIRST and refuses a corrupt row_count before a byte is + * allocated. This one is the backstop for a descriptor that + * claims billions of values to match. + */ + if (need > (uint64) MaxAllocSize) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("columnar row group " UINT64_FORMAT " declares an implausible row count", + rg->groupNumber), + errdetail("Row count " UINT64_FORMAT " would need " UINT64_FORMAT " bytes of validity bits.", + rg->rowCount, need))); + + allPresentBits = (char *) MemoryContextAlloc(rs->groupContext, + need > 0 ? (Size) need : 1); + memset(allPresentBits, 0xFF, need > 0 ? (Size) need : 1); + } + rs->nativeValidity[cc->columnIndex] = allPresentBits; + } if (cc->encodingDescriptorLen == 1 && (uint8) cc->encodingDescriptor[0] == COLUMNAR_NATIVE_ENCDESC_BASELINE) @@ -2730,7 +2863,7 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) /* D2b baseline: raw present values follow the validity bitmap; no * per-vector structure, so per-vector skipping is disabled below. The * value region runs from the bitmap end to the chunk end. */ - rs->nativeValueCursor[cc->columnIndex] = base + validityBytes; + rs->nativeValueCursor[cc->columnIndex] = base + chunkValidityBytes; rs->nativeValueEnd[cc->columnIndex] = base + cc->pageLength; allDescriptor = false; } @@ -2744,9 +2877,9 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) /* D4: reconstruct the raw present-value stream from the descriptor */ rs->nativeValueCursor[cc->columnIndex] = - pgcolumnar_native_decode_chunk(rs->groupContext, att, base + validityBytes, + pgcolumnar_native_decode_chunk(rs->groupContext, att, base + chunkValidityBytes, pgcolumnar_chunk_value_bytes(cc->pageLength, - validityBytes, + chunkValidityBytes, cc->columnIndex + 1), cc->encodingDescriptor, cc->encodingDescriptorLen, @@ -4101,6 +4234,10 @@ pgcolumnar_fetch_group_slot(uint64 storageId, uint64 groupNumber, bool *hit) * Validity bitmaps land on the fetch-cache entry. Value streams stay in * CurrentMemoryContext (the per-fetch tmp context) for the decode loop * to copy from. A column nobody projected is never read. + * + * validityBytes arrives non-negative: the caller refuses a row count whose + * bitmap would exceed MaxAllocSize, which is also what stops the int it is + * cast to from overflowing. */ static void pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, @@ -4118,6 +4255,7 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, for (c = 0; c < natts; c++) { NativeColumnChunkMetadata *cc = entry->ccForCol[c]; + int cvb; /* #1130: this chunk's validity bytes */ if (!allColumns && !bms_is_member(c, needed)) continue; @@ -4126,6 +4264,16 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, if (entry->vbits[c] != NULL && entry->rawBuf[c] != NULL) continue; + /* + * #1130: the skip below is THIS CHUNK'S bitmap against THIS CHUNK'S + * page, and eliding the bitmap is exactly what takes a well-encoded + * chunk under the group-wide size -- a sorted bigint measured 135 bytes + * of values against a 25,000-byte bitmap. Comparing against the + * group-wide size would therefore send every chunk this change helps + * most to the per-column path, which reads it a column at a time. + */ + cvb = pgcolumnar_chunk_validity_bytes(cc, validityBytes); + /* * A CHUNK THE CHECKED DECODE PATH WOULD REFUSE IS LEFT FOR IT, so * the refusal keeps its SQLSTATE. Coalescing first would span @@ -4142,8 +4290,8 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, * * Reported by @jdatcmd against #1092 + #1093 composed. */ - if (cc->pageLength < (uint64) validityBytes || - cc->pageLength - (uint64) validityBytes > (uint64) PG_UINT32_MAX) + if (cc->pageLength < (uint64) cvb || + cc->pageLength - (uint64) cvb > (uint64) PG_UINT32_MAX) continue; ranges[n].start = cc->pageOffset; @@ -4183,6 +4331,7 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, { NativeColumnChunkMetadata *cc = entry->ccForCol[c]; uint64 off; + int cvb; /* #1130: this chunk's validity bytes */ if (cc == NULL || cc->pageLength == 0) continue; @@ -4204,24 +4353,39 @@ pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry, * reads it straight from storage into an exactly-sized buffer and * refuses it there. */ - if (cc->pageLength < (uint64) validityBytes) + /* + * #1130: per chunk, because a chunk that held no null stored no + * bitmap while its neighbour in the same group may have stored one. + */ + cvb = pgcolumnar_chunk_validity_bytes(cc, validityBytes); + + if (cc->pageLength < (uint64) cvb) continue; off = cc->pageOffset - start; if (entry->vbits[c] == NULL) { MemoryContext vOld = MemoryContextSwitchTo(entry->cx); + int vbytes = validityBytes > 0 ? validityBytes : 1; - entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1); + entry->vbits[c] = palloc(vbytes); MemoryContextSwitchTo(vOld); - if (validityBytes > 0) - memcpy(entry->vbits[c], buf + off, validityBytes); + + /* + * A chunk with no stored bitmap answers "present" for every + * row, and every reader of vbits reads it as bits, so it is + * synthesized rather than special-cased at each of them. + */ + if (cvb > 0) + memcpy(entry->vbits[c], buf + off, cvb); + else + memset(entry->vbits[c], 0xFF, vbytes); } if (entry->rawBuf[c] == NULL && - cc->pageLength >= (uint64) validityBytes) + cc->pageLength >= (uint64) cvb) { - valueStream[c] = buf + off + validityBytes; - valueLen[c] = (uint32) (cc->pageLength - validityBytes); + valueStream[c] = buf + off + cvb; + valueLen[c] = (uint32) (cc->pageLength - cvb); } } @@ -4464,7 +4628,28 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, MemoryContextSwitchTo(entryOld); } - validityBytes = (int) ((entry->rowCount + 7) / 8); + /* + * SIZED IN 64-BIT AND REFUSED BEFORE THE CAST. A corrupt row_count large + * enough to overflow this int produced a NEGATIVE validityBytes, and every + * consumer below then either skipped its read and left the bits + * uninitialised or indexed past a one-byte buffer -- a hazard that predates + * #1130 and that #1130's own comments could not honestly describe, because + * they claimed a refusal further down that does not exist. Refused here + * instead, once, in the same shape as the scan path's guard, which is what + * lets every consumer below treat validityBytes as non-negative. + */ + { + uint64 need = (entry->rowCount + 7) / 8; + + if (need > (uint64) MaxAllocSize) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("columnar row group " UINT64_FORMAT " declares an implausible row count", + entry->groupNumber), + errdetail("Row count " UINT64_FORMAT " would need " UINT64_FORMAT " bytes of validity bits.", + entry->rowCount, need))); + validityBytes = (int) need; + } { char **valueStream = (char **) palloc0(sizeof(char *) * natts); @@ -4479,6 +4664,7 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, Form_pg_attribute att = TupleDescAttr(tupdesc, c); NativeColumnChunkMetadata *cc = entry->ccForCol[c]; char *vbits; + int cvb; /* #1130: this chunk's validity bytes */ char *rawBuf; uint32 rawBufLen = 0; /* byte length of rawBuf, for bounds checks */ char *cursor; @@ -4504,20 +4690,38 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, continue; } + /* + * #1130: PER CHUNK. This path is where an elided bitmap lands most + * often, because the coalesced read above skips a chunk smaller than + * the group's bitmap and eliding it is what makes a well-encoded chunk + * that small. Reading the group-wide size here took the chunk's first + * encoded bytes for a bitmap: measured on this tree before the fix, + * native_index's point lookup returned NO ROW for a row that is there. + */ + cvb = pgcolumnar_chunk_validity_bytes(cc, validityBytes); + /* * The validity bitmap, read once for this column and then kept (#433). * It is small and it is consulted on every fetch, so an overflowed * column still answers "is this row null" without touching the group. + * + * A chunk that stored none answers "present" for every row, and the + * bits are read by the rank prefix and by the null test below, so it is + * synthesized once into the same shape rather than special-cased at + * each reader. */ if (entry->vbits[c] == NULL) { MemoryContext vOld = MemoryContextSwitchTo(entry->cx); + int vbytes = validityBytes > 0 ? validityBytes : 1; - entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1); + entry->vbits[c] = palloc(vbytes); MemoryContextSwitchTo(vOld); - if (validityBytes > 0) + if (cvb > 0) PgColumnarReadLogicalData(rel, cc->pageOffset, entry->vbits[c], - validityBytes); + cvb); + else + memset(entry->vbits[c], 0xFF, vbytes); } vbits = entry->vbits[c]; if (((vbits[rowInGrp >> 3] >> (rowInGrp & 7)) & 1) == 0) @@ -4552,7 +4756,7 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, MemoryContext decCx; MemoryContext decOld; uint32 vlen = pgcolumnar_chunk_value_bytes(cc->pageLength, - validityBytes, + cvb, c + 1); char *vstream; @@ -4576,7 +4780,7 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, memcpy(vstream, valueStream[c], vlen); else PgColumnarReadLogicalData(rel, - cc->pageOffset + validityBytes, + cc->pageOffset + cvb, vstream, vlen); } diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index ce5669b4..06c231ed 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -1106,6 +1106,8 @@ flush_one_column(Form_pg_attribute att, List *chunkGroups, ListCell *lc; uint8 *validity = (uint8 *) palloc0(validityBytes); uint64 rowIdx = 0; + uint64 presentCount = 0; /* #1130: rows this chunk actually holds */ + uint8 descFlags = 0; StringInfo encoded = makeStringInfo(); StringInfo desc = makeStringInfo(); StringInfo rawRegion = makeStringInfo(); /* #1132: the unencoded alternative */ @@ -1153,13 +1155,36 @@ flush_one_column(Form_pg_attribute att, List *chunkGroups, for (i = 0; i < group->rowCount; i++, rowIdx++) if (existsBytes[i]) + { validity[rowIdx >> 3] |= (uint8) (1 << (rowIdx & 7)); + presentCount++; + } } - appendBinaryStringInfo(chunk, (char *) validity, validityBytes); + + /* + * #1130: a chunk with no nulls does not store its bitmap. + * + * The bitmap is one bit per row and is written HERE, before the block codec + * below, which therefore never compresses it -- so an all-present column + * stores ceil(rowCount / 8) bytes of 0xFF forever. Measured on a + * 1,000,000-row ClickBench table with no null in any of its 105 columns, + * that was 16.80% of everything stored; on a column that encodes well it + * reaches 99.5% of the page. + * + * DECIDED FROM presentCount, not from the attribute's NOT NULL flag. What + * matters is whether this chunk actually holds a null, which is a property + * of the rows written; a nullable column whose rows happen to be complete + * gets the saving too, and a NOT NULL constraint added later cannot make an + * already-written chunk lie. + */ + if (presentCount == rowCount) + descFlags |= COLUMNAR_ENCDESC_FLAG_NO_VALIDITY; + else + appendBinaryStringInfo(chunk, (char *) validity, validityBytes); /* descriptor header (columnar_encdesc.h owns the wire layout) */ - PgColumnarEncdescPutHeader(desc, vectorCount); - PgColumnarEncdescPutHeader(rawDesc, vectorCount); + PgColumnarEncdescPutHeaderFlags(desc, vectorCount, descFlags); + PgColumnarEncdescPutHeaderFlags(rawDesc, vectorCount, descFlags); /* * E3b: build one FSST symbol table for the whole column chunk from a @@ -1412,7 +1437,8 @@ flush_one_column(Form_pg_attribute att, List *chunkGroups, } /* - * E3b: trailing chunk-shared FSST table region (descriptor version 2). + * E3b: trailing chunk-shared FSST table region (added at descriptor version + * 2, unmoved by version 3, which spends the header's reserved byte). * sharedTableLen is 0 when the chunk has no shared table; FSST vectors * above reference this one table instead of embedding their own. */ diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index a4d6418b..2db47093 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1416,3 +1416,28 @@ projection_scan_cost projection_scan_cost premise: without the projection scan, projection_scan_cost projection_scan_cost premise: without the projection scan, the tight plan is a base columnar scan 15;16;17;18;19 never - projection_scan_cost projection_scan_cost tight and loose covering scans are not both priced at half the base 15;16;17;18;19 2026-09-17 - projection_scan_cost projection_scan_cost while a plain range on the sort key still earns its discount 15;16;17;18;19 never - +validity_elision validity_elision a chunk claiming no bitmap while it holds fewer values than rows is refused 15;16;17;18;19 never - +validity_elision validity_elision a column with no nulls stores no validity bitmap 15;16;17;18;19 2026-09-18 writer: flush_one_column's presentCount == rowCount forced false, so the bitmap is written even when the chunk holds no null +validity_elision validity_elision a null-free column elides its bitmap beside a null-bearing one in the same row group 15;16;17;18;19 never - +validity_elision validity_elision an implausible row count on an elided chunk is refused before anything is allocated 15;16;17;18;19 never - +validity_elision validity_elision and the backend survives that refusal 15;16;17;18;19 never - +validity_elision validity_elision and the backend survives that refusal too 15;16;17;18;19 never - +validity_elision validity_elision premise: each fixture is a single row group, which is what the expected size assumes 15;16;17;18;19 never - +validity_elision validity_elision premise: the arms below reach the row through an index scan 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision premise: the block codec is off, so the residual is the bitmap and nothing else 15;16;17;18;19 never - +validity_elision validity_elision premise: the first fixture holds no nulls at all 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision premise: the join arms below reach the columnar side by index too 15;16;17;18;19 never - +validity_elision validity_elision premise: the per-chunk arm's residual was summed over chunks that exist 15;16;17;18;19 never - +validity_elision validity_elision premise: the residual was summed over chunks that exist 15;16;17;18;19 never - +validity_elision validity_elision premise: the second fixture holds nulls, so its bitmap is load-bearing 15;16;17;18;19 never - +validity_elision validity_elision single-row fetches of the null-bearing column return its nulls as nulls 15;16;17;18;19 never - +validity_elision validity_elision single-row fetches through the elided path return the row asked for 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write;reader: pgcolumnar_fetch_get_row uses the group-wide validityBytes instead of this chunk's own, which is the pre-fix defect +validity_elision validity_elision the full column fetched by index matches the heap, row for row 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write;reader: pgcolumnar_fetch_get_row uses the group-wide validityBytes instead of this chunk's own, which is the pre-fix defect +validity_elision validity_elision the full column holds no row the heap does not 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision the full column preserves its null count 15;16;17;18;19 never - +validity_elision validity_elision the full column reads back exactly what the heap holds 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision the nulls column fetched by index matches the heap, row for row 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write;reader: pgcolumnar_fetch_get_row uses the group-wide validityBytes instead of this chunk's own, which is the pre-fix defect +validity_elision validity_elision the nulls column holds no row the heap does not 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision the nulls column preserves its null count 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision the nulls column reads back exactly what the heap holds 15;16;17;18;19 2026-09-18 reader: PgColumnarEncdescOmitsValidity returns false, so every reader expects a bitmap the writer did not write +validity_elision validity_elision while a column with nulls still stores one, sized one bit per row 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index bc5070c8..668942e1 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -128,4 +128,27 @@ suites_not_covered 249 # one short. The census command above was never affected because it skips nothing, # but a row count taken the other way disagrees with the tool's own `ledger: rows=` # and reads as an off-by-one in the merge rather than in the command. -checks_never_observed_red 1407 +# 1407 -> 1421 when validity_elision's twenty-five checks landed (#1130). +# Twenty-five rows, of which ELEVEN carry an observed red: the three mutations +# that ran were the writer's elision decision, the reader's flag read, and the +# fetch path's per-chunk size, and between them they reddened eleven. The +# fourteen left never are the premises, the per-chunk arm, the null-column probe +# and the two corruption guards -- those landed after the mutations ran, and the +# guards are exercised by a poisoned catalog rather than by a mutation of the +# code. The twenty-fifth is the premise @OffgridwithJD's review added: it is +# observed red by pointing the per-chunk residual at a column that does not +# exist, which is a mutation of the SUITE rather than of the product, so it is +# recorded here in prose rather than as a last-red date. +# Seeded from one run per major, all five merged in a SINGLE call, so every row +# carries 15;16;17;18;19 and the short-major warning (#1071) stayed silent. +# +# `suites_not_covered` does NOT move: registering validity_elision takes the +# registered count 260 -> 261 and seeding it takes the covered count 11 -> 12, so +# the difference is 249 either way. That is the whole reason the rows are seeded +# in this change rather than after it -- registering a suite without seeding it +# raises a number the gate may only see fall. +# +# Re-derived by COUNTING on this tree, never by adding fourteen to a number from +# another: +# awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l +checks_never_observed_red 1421 diff --git a/test/encode_post_codec.sh b/test/encode_post_codec.sh index 3f14feaf..a6502611 100755 --- a/test/encode_post_codec.sh +++ b/test/encode_post_codec.sh @@ -72,8 +72,18 @@ encoded_vectors() { # table -> count of non-NONE vectors # bitmap, which is one bit per row and is written raw ahead of the codec # (columnar_write_state.c:1079). Subtracted so the number moves only with the # encoding decision, which is what this suite is about. +# +# SUBTRACTED ONLY WHERE THERE IS ONE (#1130). A chunk that holds no null stores +# no bitmap and sets bit 0 of the descriptor's flags byte, so the old +# unconditional subtraction now removes bytes that were never written and +# understates the value stream. These fixtures hold no nulls at all, so every +# chunk takes the zero branch today; the condition is here because a fixture +# that gains a null must not silently change what this measures. value_bytes() { # table -> bytes - q "SELECT coalesce(sum(c.page_length) - sum((c.value_count + 7) / 8), 0) + q "SELECT coalesce(sum(c.page_length) - sum( + CASE WHEN octet_length(c.encoding_descriptor) >= 6 + AND (get_byte(c.encoding_descriptor, 1) & 1) = 1 + THEN 0 ELSE (c.value_count + 7) / 8 END), 0) FROM pgcolumnar.column_chunk c JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id WHERE s.relation_oid = '$1'::regclass diff --git a/test/fsst_margin.sh b/test/fsst_margin.sh index 99e26dca..4522dbb8 100755 --- a/test/fsst_margin.sh +++ b/test/fsst_margin.sh @@ -34,7 +34,8 @@ ROWS="${PGC_FSST_MARGIN_ROWS:-20000}" # How many vectors of the text column chose FSST (encoding type 8). # -# The descriptor is a 6-byte header -- version, a reserved byte, then the vector +# The descriptor is a 6-byte header -- version, a flags byte (#1130; a reserved +# zero before it), then the vector # count as uint32 -- followed by that many 13-byte entries. Same decode as # write_fsst_compressed.sh, and for the same reason: reading past the entries # would score the chunk's symbol table as encoding types. diff --git a/test/fsst_verdict_cache.sh b/test/fsst_verdict_cache.sh index 59f8db44..7fe6f157 100755 --- a/test/fsst_verdict_cache.sh +++ b/test/fsst_verdict_cache.sh @@ -76,7 +76,8 @@ fingerprint() { # fingerprint # 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 +# flags byte -- a reserved zero before #1130 -- 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. diff --git a/test/native_encdesc_golden.sh b/test/native_encdesc_golden.sh index 3c787c71..5d44845d 100755 --- a/test/native_encdesc_golden.sh +++ b/test/native_encdesc_golden.sh @@ -18,8 +18,14 @@ # production. It does NOT pin which encoding was chosen, so encoder tuning does # not spuriously break it. # -# Layout (descriptor version 2): -# header: version u8 @0, reserved u8 @1, vectorCount u32 @2 (HEADER_LEN 6) +# Layout (descriptor version 3): +# header: version u8 @0, flags u8 @1, vectorCount u32 @2 (HEADER_LEN 6) +# +# VERSION 3 SPENDS THE RESERVED BYTE ON FLAGS (#1130). Bit 0, NO_VALIDITY, says +# the chunk held no nulls and therefore did NOT store its validity bitmap, so +# the page is [encoded] rather than [validity][encoded]. Every other field keeps +# its offset, which is why a version-2 descriptor is readable as a version-3 one +# whose flags are clear -- and why this suite's offsets below did not move. # entry: type u8 @0, valueCount u32 @1, rawLen u32 @5, encLen u32 @9 (ENTRY_LEN 13) # trailer: sharedTableLen u32, then that many bytes # @@ -50,9 +56,19 @@ le32() { q "SELECT get_byte(d,$2)+get_byte(d,$2+1)*256+get_byte(d,$2+2)*65536 olen() { q "SELECT octet_length(encoding_descriptor) FROM pgcolumnar.column_chunk WHERE storage_id=$SID AND column_index=$1;"; } -# version byte is 2 on every column's descriptor +# version byte is 3 on every column's descriptor +for col in 0 1 2; do + check "column $col descriptor version byte is 3" "$(b $col 0)" "3" +done + +# FLAGS (u8 @1), which version 2 wrote as a zero reserved byte. This fixture +# inserts no NULL, so every column asserts NO_VALIDITY and stores no bitmap. +# Pinned per column rather than once, because the flag is a property of the +# CHUNK: one column may hold a null while its neighbour does not, and a reader +# that decided this per row group would be wrong for exactly that table. for col in 0 1 2; do - check "column $col descriptor version byte is 2" "$(b $col 0)" "2" + check "column $col descriptor flags byte says the bitmap was omitted" \ + "$(b $col 1)" "1" done # vectorCount (u32 @2) is 1 (one chunk group, one vector) diff --git a/test/native_fetch_coalesce.sh b/test/native_fetch_coalesce.sh index 1aa1b686..e6a44177 100755 --- a/test/native_fetch_coalesce.sh +++ b/test/native_fetch_coalesce.sh @@ -120,7 +120,7 @@ check "premise: the wide fetch returns the projected values" \ # ---- the validity copy must be bounded by the chunk, not by the row count ----- # # THIS IS AN ORDERING PIN AND IT IS DELIBERATELY NOT BEHAVIOURAL. The defect it -# guards was a heap overread: the coalesced path copies validityBytes out of a +# guards was a heap overread: the coalesced path copies the validity bytes out of a # span buffer that is only guaranteed to hold page_length bytes for this chunk, # and the test reconciling the two used to run three lines AFTER the copy. # @@ -149,11 +149,18 @@ SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" # $2 is an OPTIONAL anchor: the search starts only after a line matching it. # THE FUNCTION HAS TWO GUARDS WITH THE SAME TEXT. The range-building loop defers # a chunk the checked decode path would refuse, and the distribution loop bounds -# the validity copy; both read `pageLength < (uint64) validityBytes`. Without an -# anchor this finds the FIRST, which is in the wrong loop -- the ordering check -# would then still pass with the distribution guard deleted, which is the guard -# it exists to pin. Anchoring on the containment test, which only the -# distribution loop has, pins the right one. Reported by @jdatcmd. +# the validity copy; both read `pageLength < (uint64) cvb`. Without an anchor +# this finds the FIRST, which is in the wrong loop -- the ordering check would +# then still pass with the distribution guard deleted, which is the guard it +# exists to pin. Anchoring on the containment test, which only the distribution +# loop has, pins the right one. Reported by @jdatcmd. +# +# THE TEXT MOVED WITH #1130 and the pattern moved with it: both guards compared +# against `validityBytes`, the row group's ceil(rowCount / 8), until the bitmap +# became a property of the CHUNK. `cvb` is that chunk's own size, 0 where the +# bitmap was elided. The property this suite pins -- the bound precedes the copy +# -- is unchanged; only the name it is spelled with moved, which is the standing +# cost of pinning source text and the reason the pattern is stated once here. _nfc_line() { awk -v pat="$1" -v after="$2" ' /^pgcolumnar_fetch_coalesce_read[(]/ { f = 1 } @@ -179,7 +186,7 @@ _nfc_line() { # here and failed there. A bracket expression cannot be mangled by -v escape # processing and means a literal paren in both. Verified identical under mawk # and gawk. Reported by @jdatcmd. -_nfc_guard="$(_nfc_line 'pageLength < [(]uint64[)] validityBytes' 'cc->pageOffset < start')" +_nfc_guard="$(_nfc_line 'pageLength < [(]uint64[)] cvb' 'cc->pageOffset < start')" _nfc_copy="$(_nfc_line 'memcpy[(]entry->vbits' '')" check "premise: the coalescing helper holds both the bound and the validity copy" \ diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 9f73d87c..4d86c99e 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -102,6 +102,7 @@ behaviour, the source of that number is named. - [54. test_projection_update.py: UPDATE must fan the new row number out to projections](#54-test_projection_updatepy-update-must-fan-the-new-row-number-out-to-projections) - [55. test_projection_drop_column.py: DROP COLUMN must not invalidate a projection](#55-test_projection_drop_columnpy-drop-column-must-not-invalidate-a-projection) - [56. test_encode_post_codec.py: an encoding must be smaller after the codec](#56-test_encode_post_codecpy-an-encoding-must-be-smaller-after-the-codec) +- [57. test_validity_elision.py: a column with no nulls must store no validity bitmap](#57-test_validity_elisionpy-a-column-with-no-nulls-must-store-no-validity-bitmap) ## 1. How to read a test in here @@ -4754,3 +4755,99 @@ The controls are the load-bearing half. Declining every encoding would satisfy "never larger than unencoded" and redden nothing, so the repeating column ships beside the tail one and asserts that encoding is still chosen where it wins. +## 57. test_validity_elision.py: a column with no nulls must store no validity bitmap + +#1130. A column chunk's page was always `[validity bitmap][encoded values]`. The +bitmap is one bit per row, written RAW ahead of the block codec, which therefore +never saw it -- so a column holding no null still stored `ceil(rows / 8)` bytes +of `0xFF` for ever. Measured on ClickBench `hits_0.parquet` (1,000,000 rows, 105 +columns, no null in any of them): **16.80% of everything stored**, and 99.5% of +the page on a well-encoded column whose values came to 135 bytes. + +The writer now omits the bitmap for a chunk that holds no null and says so in the +descriptor's new flags byte (`NO_VALIDITY`, descriptor version 3). + +**THE MEASUREMENT IS EXACT, not a ratio.** With the block codec off the page is +exactly `[validity][encoded]`, so `page_length - sum(encLen)` IS the bitmap, with +nothing else in it. The arms assert `0` and `ceil(rows / 8)` rather than a +threshold, so a wrong answer cannot hide inside a tolerance. The suite turns the +codec off for that reason; with one, the same subtraction reads a compression +ratio and calls it a bitmap. + +**THE FETCH PATH IS A SECOND READER, and it is the reason this file has a third +test.** A scan reaches a chunk through `pgcolumnar_native_load_group`; an index +scan reaches a row through `pgcolumnar_fetch_get_row`, which reads the chunk's +bytes itself. The first implementation fixed the scan and not the fetch, and the +PG17 matrix went red in 33 suites -- `native_index`'s point lookup returned NO +ROW for a row that is there. Worse, the two are CORRELATED: the coalesced read +skips a 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. A fixture built +to exercise the elision is therefore systematically the fixture that lands on +the reader most likely to have been left behind. + +**TWO ARRANGEMENTS MAKE THE FETCH ARMS REAL, and both were found by an arm that +failed rather than by reasoning.** The table carries a KEY column beside the +measured one, because an index on the only column is answered by an Index Only +Scan that never calls the table AM at all -- the first version of these arms +passed against a build whose fetch path was provably broken. And +`pgcolumnar.enable_custom_scan` is off, because `enable_seqscan` does not govern +the columnar custom scan: without it the plan was +`Custom Scan (PgColumnarScan)`, a scan wearing a fetch's name. The premise arm +asserts the PLAN NODE rather than the row, because a row that comes back is no +evidence about which reader produced it. + +Public seam: `pgcolumnar.column_chunk` and the encoding descriptor. Independent +of `test/validity_elision.sh`, which reads the descriptor through `get_byte()` +in SQL and turns the codec off with `ALTER DATABASE`, while this decodes the +descriptor in Python bytes and uses a session `SET` -- which it can, because one +connection serves the whole test and the shell twin's `psql_run` opens a new +session per statement. Neither file names the other. + +### Every arm + +| test | what it holds | +| --- | --- | +| `test_a_column_with_no_nulls_stores_no_validity_bitmap` | the size arms and the five premises they rest on | +| `test_the_layout_change_returns_the_same_rows` | the invariant the size arms exist to prove is not vacuous | +| `test_the_guards_the_elision_added_refuse_a_lying_catalog` | both new guards, poisoned into firing, asserted by SQLSTATE | +| `test_the_fetch_path_reads_an_elided_chunk_correctly` | the second reader, with the plan asserted rather than assumed | + +**FIVE PREMISES, AND EACH ANSWERS A WAY THE HEADLINE ARM READS 0 WITHOUT +MEASURING ANYTHING.** The residual arm expects `0`, which is also what +`coalesce(sum(...), 0)` returns over an empty set, so the chunk count is +asserted beside it. The exact size assumes one row group, so the group count is +asserted. And the subtraction is the bitmap only while the block codec is off, +so the setting is read back rather than assumed — `SET` and `ALTER DATABASE` +are statements that have to have taken effect. + +**THE PER-CHUNK ARM IS THE ONE THAT CANNOT BE PASSED BY A PER-GROUP DECISION.** +Both size arms are satisfied by a writer that decides elision once per row +group: one fixture's group holds no null anywhere and the other's holds some. +The null-bearing fixture carries a null-free KEY column in the same row group, +so only a per-chunk decision elides one and keeps the other. + +**AND IT CARRIES ITS OWN PREMISE, BESIDE IT RATHER THAN WITH THE OTHERS.** It +expects `0`, and `0` is also what the residual returns when it summed nothing: +a column index off the end of the table answers exactly as a correctly elided +bitmap does. @OffgridwithJD measured that against the first version of this +pair -- pointing that one residual at `column_index 99` left all 24 checks green +in BOTH harnesses, because `_residual` returns the chunk count and two of its +three call sites discarded it. The two residuals that keep discarding it are the +ones a bug makes LARGE, which is the distinction worth carrying: a premise is +owed wherever the failure mode and the pass look the same. + +**THE GUARD ARMS ASSERT SQLSTATE `XX001`**, not message text: that is +`ERRCODE_DATA_CORRUPTED` and comes only from a guard that ran. One poisons the +`NO_VALIDITY` flag onto a chunk that holds nulls; the other poisons the row +count. Both also assert the backend survived. The second arm records a refuted +idea as well: bounding the synthesized bitmap by the row group's byte length +looks right and is wrong, because an elided group of 360 bytes legitimately +needs 12,500 bytes of bits — which is the saving, not a defect. It was measured +when that bound made a plain `SELECT` raise on a correct table. + +The second fixture is the load-bearing control. Deleting the bitmap +unconditionally satisfies "a column with no nulls stores none" and loses every +null in the table, so the column that still needs its bitmap is asserted beside +the one that does not -- and the null count is asserted separately, because both +`EXCEPT ALL` arms are satisfied by a table that agrees on values and not on +nulls. diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 2e441d7c..1d5f79ba 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -310,4 +310,12 @@ guard_tests 380 # `guard_tests` was re-derived in the same run and did NOT move: 380. That is the # expected answer for a file that needs a cluster, and checking it rather than # assuming it is what this file asks for. -cluster_tests 423 +# 423 -> 427 when test_validity_elision.py landed (#1130): four cluster arms over +# the elided validity bitmap -- the size arms with their premises, the read-back +# invariant they exist to prove is not vacuous, the FETCH path, which is a second +# reader with its own copy of the layout, and the two corruption guards the +# elision added, each poisoned into firing and asserted by SQLSTATE. +# +# DERIVED BY COLLECTION on this tree: `427 tests collected`. `guard_tests` was +# re-derived in the same run and did NOT move: 380. +cluster_tests 427 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 160021ad..da91562f 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -95,7 +95,7 @@ "projections", "sorted_pathkeys", "stats_privilege", "index_fetch_penalty_crossover", - "parallel_scan_cost", "zonemap_boundaries"] + "parallel_scan_cost", "validity_elision", "zonemap_boundaries"] # stem -> why it does not yet reach zero. Empty today, and an entry here is a claim # about the PORT rather than a licence: the standing arm does not grade it, so the diff --git a/test/pytest/test_compression_reaches_the_cascade.py b/test/pytest/test_compression_reaches_the_cascade.py index d7021365..31cf0edd 100644 --- a/test/pytest/test_compression_reaches_the_cascade.py +++ b/test/pytest/test_compression_reaches_the_cascade.py @@ -36,7 +36,8 @@ def _fsst_vectors(cur, table): """How many vectors of column 1 chose FSST, read from the descriptor. - The descriptor is a 6-byte header -- version, a reserved byte, then the vector + The descriptor is a 6-byte header -- version, a flags byte (#1130; a + reserved zero before it), then the vector count as uint32 little-endian -- followed by that many 13-byte entries whose first byte is the encoding type. Reading past the entries would score the chunk's shared symbol table bytes as encoding types, so the count bounds the diff --git a/test/pytest/test_encode_post_codec.py b/test/pytest/test_encode_post_codec.py index 8743881d..ba64f05e 100644 --- a/test/pytest/test_encode_post_codec.py +++ b/test/pytest/test_encode_post_codec.py @@ -39,10 +39,11 @@ def _descriptor_encodings(cur, table): """Encoding type of every vector of column 0, read from the descriptor. - 6-byte header -- version, a reserved byte, then the vector count as uint32 - little-endian -- followed by that many 13-byte entries whose first byte is - the encoding type. The count bounds the scan: reading to the descriptor's - length would score the trailing shared-table region as encoding types. + 6-byte header -- version, a flags byte (a reserved zero before #1130), then + the vector count as uint32 little-endian -- followed by that many 13-byte + entries whose first byte is the encoding type. The count bounds the scan: + reading to the descriptor's length would score the trailing shared-table + region as encoding types. """ cur.execute( """ @@ -72,10 +73,19 @@ def _value_bytes(cur, table): The bitmap is one bit per row and is written raw ahead of the codec, so it is subtracted to leave a number that moves only with the encoding decision. + + SUBTRACTED ONLY WHERE THERE IS ONE (#1130). A chunk holding no null stores no + bitmap and sets bit 0 of the descriptor's flags byte; subtracting + unconditionally would remove bytes that were never written. These fixtures + hold no nulls, so every chunk takes the zero branch today -- the condition is + here so a fixture that gains one cannot silently change what is measured. """ cur.execute( """ - SELECT coalesce(sum(c.page_length) - sum((c.value_count + 7) / 8), 0) + SELECT coalesce(sum(c.page_length) - sum( + CASE WHEN octet_length(c.encoding_descriptor) >= 6 + AND (get_byte(c.encoding_descriptor, 1) & 1) = 1 + THEN 0 ELSE (c.value_count + 7) / 8 END), 0) FROM pgcolumnar.column_chunk c JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id WHERE s.relation_oid = %s::regclass diff --git a/test/pytest/test_native_fetch_coalesce.py b/test/pytest/test_native_fetch_coalesce.py index 2c101cf1..c0c53f1e 100644 --- a/test/pytest/test_native_fetch_coalesce.py +++ b/test/pytest/test_native_fetch_coalesce.py @@ -143,7 +143,7 @@ def test_native_fetch_coalesce(pgc_conn, expect): ) # ---- the validity copy must be bounded by the chunk, not by the row count ---- # -# The coalesced fetch path copies validityBytes out of a span buffer that is only +# The coalesced fetch path copies the validity bytes out of a span buffer that is only # guaranteed to hold page_length bytes for the chunk being served. The test that # reconciles the two once ran AFTER the copy, which made a chunk whose catalog # page_length was under its validity bitmap read past the allocation -- measured @@ -177,8 +177,13 @@ def test_the_validity_copy_is_bounded_before_the_chunk_is_read(expect): # in the wrong loop -- this check would then still pass with the # distribution guard deleted. The containment test belongs only to the # distribution loop, so searching after it pins the right guard. + # + # THE TEXT MOVED WITH #1130: both guards compared against `validityBytes`, + # the row group's ceil(rowCount / 8), until the bitmap became a property of + # the CHUNK. `cvb` is that chunk's own size, 0 where it was elided. The + # property pinned here is unchanged; only its spelling moved. anchor = body.find("cc->pageOffset < start") - guard = body.find("pageLength < (uint64) validityBytes", anchor + 1) if anchor >= 0 else -1 + guard = body.find("pageLength < (uint64) cvb", anchor + 1) if anchor >= 0 else -1 copy = body.find("memcpy(entry->vbits") expect.num( diff --git a/test/pytest/test_validity_elision.py b/test/pytest/test_validity_elision.py new file mode 100644 index 00000000..1851fbe6 --- /dev/null +++ b/test/pytest/test_validity_elision.py @@ -0,0 +1,397 @@ +"""A column with no nulls must not store a validity bitmap (#1130). + +`flush_one_column` builds the page as `[validity][finalData]`. The bitmap is one +bit per row, allocated unconditionally and appended RAW, ahead of the block +codec, which therefore never sees it -- so a column that holds no null at all +still stored `ceil(rows / 8)` bytes of `0xFF` for ever. + +Measured on ClickBench `hits_0.parquet` (1,000,000 rows, 105 columns, no null in +any of them): 16.80% of everything stored. On a column that encodes well it +dominates the page -- 99.5% on a sorted bigint, where the values themselves came +to 135 bytes. + +THE MEASUREMENT IS EXACT, not a ratio. With the block codec off the page is +exactly `[validity][encoded]`, so + + page_length - sum(encLen over the chunk's vectors) + +IS the bitmap, with nothing else in it. So the arms below assert 0 and +`ceil(rows / 8)`, and a wrong answer cannot hide inside a tolerance. + +Independent of `test/validity_elision.sh` per CONTEXT.md: the same public seams +-- `pgcolumnar.column_chunk` and the encoding descriptor -- but its own cluster, +its own table names, its own corpus size, and the descriptor decoded here in +Python bytes rather than through `get_byte()` in SQL. The codec is turned off +here with a session `SET`, which this harness can rely on because one connection +serves the whole test; the shell twin cannot, and uses `ALTER DATABASE`. The two +agree on the property, not on the implementation. + +THE FETCH PATH IS A SECOND READER and it gets its own arms. A scan reaches a +chunk through `pgcolumnar_native_load_group`; an index scan reaches a row +through `pgcolumnar_fetch_get_row`, which reads the chunk's bytes itself. +Elision makes that path MORE likely for the columns it helps most: the coalesced +read skips a chunk whose `page_length` is below the group's bitmap size, and +dropping the bitmap is exactly what takes a well-encoded chunk below it. So a +fixture built to exercise the elision is systematically the fixture that lands +on the reader most likely to be left unpatched -- which is what happened here: +the first implementation fixed the scan path only, and 33 suites went red. + +TWO ARRANGEMENTS MAKE THE FETCH ARMS REAL, and both were found by an arm that +failed rather than by reasoning: + + * the table has a KEY column beside the measured one, because an index on the + only column is answered by an Index Only Scan that never calls the table AM; + * `pgcolumnar.enable_custom_scan` is off, because `enable_seqscan` does not + govern the columnar custom scan and the planner otherwise answers from + `Custom Scan (PgColumnarScan)` -- a scan, not a fetch. + +The premise arm asserts the plan, not the row, for that reason: a row that comes +back says nothing about which reader produced it. +""" + +ROWS = 80000 # own corpus size; the shell twin uses another +NULL_EVERY = 10 # one row in this many is NULL in the second fixture + +HEADER_LEN = 6 # version u8, flags u8, vectorCount u32 +ENTRY_LEN = 13 # type u8, valueCount u32, rawLen u32, encLen u32 +OFF_ENCLEN = 9 # within an entry + +MEASURED_COLUMN = 1 # v, the column whose null-ness differs; 0 is the key +KEY_COLUMN = 0 # k, null-free in both fixtures + + +def _residual(cur, table, column=MEASURED_COLUMN): + """`page_length - sum(encLen)` over one column's chunks, and how many. + + With the block codec off that difference IS the validity bitmap. The vector + count bounds the walk: reading entries to the descriptor's length would + score the trailing shared-table region as vectors and subtract bytes that + are not there. + + THE COUNT IS RETURNED BESIDE THE SUM because a sum over nothing is 0, which + is exactly what the headline arm wants to see. A residual of 0 is evidence + only together with the number of chunks it was summed over. + + The column is an argument because the property is per CHUNK, and the + sharpest statement of that is two columns of one row group answering + differently. + """ + cur.execute( + """ + SELECT c.page_length, c.encoding_descriptor + FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = %s::regclass + AND c.column_index = %s + """, + (table, column), + ) + total = 0 + chunks = 0 + for page_length, desc in cur.fetchall(): + blob = bytes(desc) + if len(blob) < HEADER_LEN: + continue + chunks += 1 + count = int.from_bytes(blob[2:HEADER_LEN], "little") + enc = 0 + for i in range(count): + at = HEADER_LEN + i * ENTRY_LEN + OFF_ENCLEN + if at + 4 <= len(blob): + enc += int.from_bytes(blob[at:at + 4], "little") + total += int(page_length) - enc + return total, chunks + + +def _row_groups(cur, table): + cur.execute( + """ + SELECT count(*) FROM pgcolumnar.row_group r + JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id + WHERE s.relation_oid = %s::regclass + """, + (table,), + ) + return int(cur.fetchone()[0]) + + +def _load(cur): + """Both fixtures, and their heap mirrors, with the block codec off. + + NO CODEC, deliberately. With one, `page_length` is the COMPRESSED encoded + region and the subtraction above stops being the bitmap size: the arms would + then be reading a compression ratio and calling it a bitmap. + """ + cur.execute("SET pgcolumnar.compression = 'none'") + + cur.execute("CREATE TABLE vb_full (k bigint, v bigint) USING pgcolumnar") + cur.execute(f"INSERT INTO vb_full SELECT g, g FROM generate_series(1, {ROWS}) g") + cur.execute("CREATE TABLE vb_full_h AS SELECT * FROM vb_full") + + cur.execute("CREATE TABLE vb_nulls (k bigint, v bigint) USING pgcolumnar") + cur.execute( + f"INSERT INTO vb_nulls SELECT g, CASE WHEN g % {NULL_EVERY} = 0 THEN NULL " + f"ELSE g END FROM generate_series(1, {ROWS}) g" + ) + cur.execute("CREATE TABLE vb_nulls_h AS SELECT * FROM vb_nulls") + + +def _count(cur, sql, args=()): + cur.execute(sql, args) + return int(cur.fetchone()[0]) + + +def test_a_column_with_no_nulls_stores_no_validity_bitmap(pgc_conn, expect): + """The size arms, with the premise each of them rests on. + + The second fixture is not decoration. Deleting the bitmap unconditionally + satisfies the first arm and loses every null in the table, so the column + that still needs its bitmap is asserted beside the one that does not. + """ + with pgc_conn.cursor() as c: + _load(c) + + full_res, full_chunks = _residual(c, "vb_full") + nulls_res, _ = _residual(c, "vb_nulls") + key_res, key_chunks = _residual(c, "vb_nulls", KEY_COLUMN) + + # THE PREMISE THE EXACT NUMBER RESTS ON. With a block codec, + # page_length is the COMPRESSED encoded region and the subtraction stops + # being the bitmap: the arms would read a compression ratio and call it + # a bitmap. Read back rather than assumed -- the SET is a statement that + # has to have taken effect. + c.execute("SHOW pgcolumnar.compression") + expect.text( + c.fetchone()[0], "none", + "premise: the block codec is off, so the residual is the bitmap and nothing else", + ) + + expect.text( + "measured" if full_chunks > 0 else "measured-nothing", "measured", + "premise: the residual was summed over chunks that exist", + ) + + # ceil(ROWS / 8) is ONE group's bitmap. A fixture split across two groups + # stores two of them, and would match the expected total only by an + # accident of rounding. + expect.text( + f"{_row_groups(c, 'vb_full')}{_row_groups(c, 'vb_nulls')}", "11", + "premise: each fixture is a single row group, which is what the expected size assumes", + ) + + full_nulls = _count(c, "SELECT count(*) FROM vb_full WHERE v IS NULL") + expect.num(full_nulls, 0, "premise: the first fixture holds no nulls at all") + + some_nulls = _count(c, "SELECT count(*) FROM vb_nulls WHERE v IS NULL") + expect.text( + "has-nulls" if some_nulls > 0 else "none", "has-nulls", + "premise: the second fixture holds nulls, so its bitmap is load-bearing", + ) + + expect.num(full_res, 0, + "a column with no nulls stores no validity bitmap") + expect.num(nulls_res, (ROWS + 7) // 8, + "while a column with nulls still stores one, sized one bit per row") + + # ITS OWN PREMISE, BESIDE IT RATHER THAN WITH THE OTHERS, so the two + # move together. The arm below expects 0, and 0 is also what the + # residual returns when it summed nothing -- a column index off the end + # of the table answers exactly as a correctly elided bitmap does. + # Measured by @OffgridwithJD against the first version of this file: + # `KEY_COLUMN = 99` left all 24 checks green. `_residual` returns the + # count for this reason and two of its three call sites may discard it, + # because their residual is one that a bug makes LARGE. + expect.text( + "measured" if key_chunks > 0 else "measured-nothing", "measured", + "premise: the per-chunk arm's residual was summed over chunks that exist", + ) + + # THE ONLY ARM THAT SAYS THE DECISION IS PER CHUNK. Both arms above are + # satisfied by a writer deciding once per ROW GROUP: one fixture's group + # holds no null anywhere and the other's holds some. vb_nulls carries a + # null-free key column in the SAME row group as its null-bearing one. + expect.num(key_res, 0, + "a null-free column elides its bitmap beside a null-bearing one " + "in the same row group") + + +def test_the_layout_change_returns_the_same_rows(pgc_conn, expect): + """The invariant the size arms exist to protect. + + A layout change that loses or shifts a null is a data-loss bug, not a size + regression, and the null count is asserted separately because both EXCEPT + ALL arms are satisfied by a table that agrees on values and not on nulls. + """ + with pgc_conn.cursor() as c: + _load(c) + + for label, tbl in (("full", "vb_full"), ("nulls", "vb_nulls")): + expect.num( + _count(c, f"SELECT count(*) FROM (SELECT k, v FROM {tbl} " + f"EXCEPT ALL SELECT k, v FROM {tbl}_h) d"), + 0, f"the {label} column reads back exactly what the heap holds", + ) + expect.num( + _count(c, f"SELECT count(*) FROM (SELECT k, v FROM {tbl}_h " + f"EXCEPT ALL SELECT k, v FROM {tbl}) d"), + 0, f"the {label} column holds no row the heap does not", + ) + expect.num( + _count(c, f"SELECT count(*) FROM {tbl} WHERE v IS NULL"), + _count(c, f"SELECT count(*) FROM {tbl}_h WHERE v IS NULL"), + f"the {label} column preserves its null count", + ) + + +def test_the_guards_the_elision_added_refuse_a_lying_catalog(pgc_conn, expect): + """The two guards, each corrupted into firing. + + Neither is reachable from a table written correctly, so the catalog is + poisoned the way corruption.sh poisons the fields that predate this change. + + THE SQLSTATE IS THE ASSERTION, not the message. `XX001` is + ERRCODE_DATA_CORRUPTED and comes only from a guard that ran; a message grep + is equally satisfied by a missing function, a bad argument or a login + failure. + """ + import psycopg + + with pgc_conn.cursor() as c: + _load(c) + + # GUARD 1: the chunk CLAIMS it stored no bitmap while holding fewer + # values than the group has rows. vb_nulls' v column holds 90% of them, + # so setting the flag on it is exactly the lie the guard exists for. + c.execute( + """ + UPDATE pgcolumnar.column_chunk c + SET encoding_descriptor = set_byte(c.encoding_descriptor, 1, 1) + FROM pgcolumnar.storage s + WHERE s.storage_id = c.storage_id + AND s.relation_oid = 'vb_nulls'::regclass + AND c.column_index = %s + """, + (MEASURED_COLUMN,), + ) + try: + c.execute("SELECT sum(v) FROM vb_nulls") + raised = None + except psycopg.Error as exc: + raised = exc + expect.sqlstate( + raised, "XX001", + "a chunk claiming no bitmap while it holds fewer values than rows is refused", + ) + expect.num(_count(c, "SELECT 1"), 1, "and the backend survives that refusal") + + # GUARD 2: a row count the chunk cannot hold. The descriptor still + # accounts for its own rows, so the claim is false in the other + # direction and the reader refuses BEFORE synthesizing half a gigabyte + # of bits. + # + # BOUNDING THE SYNTHESIZED BITMAP BY THE ROW GROUP'S BYTE LENGTH DOES + # NOT WORK: an elided group of two well-encoded bigint columns measured + # 360 bytes on disk against the 12,500 bytes of bits it no longer + # stores. That is the saving, not a defect. + c.execute( + """ + UPDATE pgcolumnar.row_group r + SET row_count = 4000000000 + FROM pgcolumnar.storage s + WHERE s.storage_id = r.storage_id + AND s.relation_oid = 'vb_full'::regclass + """ + ) + try: + c.execute("SELECT sum(v) FROM vb_full") + raised = None + except psycopg.Error as exc: + raised = exc + expect.sqlstate( + raised, "XX001", + "an implausible row count on an elided chunk is refused before anything is allocated", + ) + expect.num(_count(c, "SELECT 1"), 1, "and the backend survives that refusal too") + + +def test_the_fetch_path_reads_an_elided_chunk_correctly(pgc_conn, expect): + """The second reader, which the scan arms above cannot speak for. + + The premise asserts the PLAN. A row that comes back is no evidence about + which reader produced it, and the two readers disagree only for a chunk that + elided its bitmap -- which is why the first version of these arms passed + against a build whose fetch path returned no row at all for a row that is + there. + """ + with pgc_conn.cursor() as c: + _load(c) + c.execute("CREATE INDEX vb_full_k ON vb_full (k)") + c.execute("CREATE INDEX vb_nulls_k ON vb_nulls (k)") + + c.execute("SET pgcolumnar.enable_custom_scan = off") + c.execute("SET enable_seqscan = off") + c.execute("SET enable_bitmapscan = off") + c.execute("SET max_parallel_workers_per_gather = 0") + + c.execute("EXPLAIN (COSTS OFF) SELECT v FROM vb_full WHERE k = 12345") + plan = [row[0] for row in c.fetchall()] + expect.num( + sum(1 for line in plan if line.lstrip().startswith("Index Scan using")), 1, + "premise: the arms below reach the row through an index scan", + ) + + # AND THE SHAPE THE NEXT ARMS ACTUALLY RUN. The premise above plans a + # point query; these plan a join, which the planner may serve + # differently. Asserting one and running the other is how an arm ends up + # licensed by a plan nobody produced. + c.execute( + "EXPLAIN (COSTS OFF) SELECT count(*) FROM vb_full c " + "JOIN vb_full_h h ON h.k = c.k WHERE c.v IS DISTINCT FROM h.v" + ) + join_plan = [row[0] for row in c.fetchall()] + expect.num( + sum(1 for line in join_plan + if line.lstrip().lstrip("-> ").startswith("Index Scan using vb_full_k")), 1, + "premise: the join arms below reach the columnar side by index too", + ) + + for label, tbl in (("full", "vb_full"), ("nulls", "vb_nulls")): + expect.num( + _count(c, f"SELECT count(*) FROM {tbl} c JOIN {tbl}_h h ON h.k = c.k " + f"WHERE c.v IS DISTINCT FROM h.v"), + 0, f"the {label} column fetched by index matches the heap, row for row", + ) + + # ONE ROW AT A TIME, at positions where a bitmap read out of the wrong + # bytes lands on a neighbour or reports a present row as NULL. The join + # above is satisfied by a fetch that returns the row it was asked for; + # these are not. + bad = [] + for pos in (1, 2, 3, 8, 9, 4097, ROWS // 2, ROWS - 1, ROWS): + c.execute("SELECT v FROM vb_full WHERE k = %s", (pos,)) + row = c.fetchone() + got = None if row is None else row[0] + if got != pos: + bad.append(f"k={pos}(got={got})") + expect.text( + " ".join(bad) if bad else "same", "same", + "single-row fetches through the elided path return the row asked for", + ) + + # THE NULL-BEARING COLUMN NEEDS ITS OWN ONE-ROW PROBE, and a null row in + # it. vb_nulls keeps its bitmap, so these exercise the other branch of + # the same per-chunk decision -- and the row that is genuinely NULL is + # the one a fetch that lost the bitmap answers wrongly in the + # safe-looking direction. + bad_n = [] + for pos in (9, 10, 11, 4096, 4097, ROWS // 2, ROWS - 1, ROWS): + want = None if pos % NULL_EVERY == 0 else pos + c.execute("SELECT v FROM vb_nulls WHERE k = %s", (pos,)) + row = c.fetchone() + got = None if row is None else row[0] + if got != want: + bad_n.append(f"k={pos}(got={got} want={want})") + expect.text( + " ".join(bad_n) if bad_n else "same", "same", + "single-row fetches of the null-bearing column return its nulls as nulls", + ) diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 1ae9071d..e929aa52 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -289,6 +289,7 @@ SUITES=( vacuum_lock_privilege vacuum_sorted_gate vacuum_stripe_count + validity_elision vector_agg_rescan_memory vector_agg_tlist_shape vm_clear_on_renumber diff --git a/test/validity_elision.sh b/test/validity_elision.sh new file mode 100755 index 00000000..7b14188f --- /dev/null +++ b/test/validity_elision.sh @@ -0,0 +1,364 @@ +#!/usr/bin/env bash +# +# A column with no nulls must not store a validity bitmap (#1130). +# +# flush_one_column builds the page as [validity][finalData] +# (columnar_write_state.c:1079). The bitmap is one bit per row, allocated +# unconditionally (:1107) and appended raw (:1156) BEFORE the block codec runs +# (:1479), which therefore never sees it. A NOT NULL column, or one that simply +# holds no nulls, still writes ceil(rows/8) bytes of 0xFF. +# +# Measured on ClickBench hits_0.parquet -- 1,000,000 rows, 105 columns, no nulls +# in any of them -- the bitmap was 16.03% of the stored table, and 16.80% after +# #1132 shrank the values around it. On a column that encodes well it dominates: +# on a sorted bigint it was 99.5% of the page. +# +# THE MEASUREMENT IS EXACT, not a ratio, which is why this suite can assert a +# number rather than a threshold. With no block codec the page is exactly +# [validity][encoded], so +# +# page_length - sum(encLen over the chunk's vectors) +# +# IS the validity size, with nothing else in it. Verified on eight chunks across +# four corpora before this change: residual 0 on every one. So the arm reads 0 +# for an elided bitmap and ceil(rows/8) for a present one, and a wrong answer +# cannot hide inside a tolerance. +# +# It asserts: +# 1. the two fixtures are what they claim -- one holds no nulls, the other +# holds some -- because the arms below are about that difference; +# 2. a column with no nulls stores no bitmap at all; +# 3. a column WITH nulls still stores one. This is the silent direction: +# deleting the bitmap unconditionally satisfies 2 and loses every null; +# 4. both columns read back exactly what the heap holds, NULLs included; +# 5. and they read back the same through the FETCH path, which is a second +# reader with its own copy of the layout. The elided chunk is the one most +# likely to land there: the coalesced read skips a chunk smaller than the +# group's bitmap, and eliding the bitmap is what takes it below that. +# +# Usage: test/validity_elision.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_VALIDITY_ROWS:-100000}" + +# page_length minus the descriptor's own accounting of the encoded bytes. +# +# Same descriptor decode as fsst_margin.sh and encode_post_codec.sh: a 6-byte +# header -- version, a flags byte, then the vector count as uint32 -- followed by +# that many 13-byte entries of [type][valueCount][rawLen][encLen]. The count +# bounds the scan, so the trailing shared-table region is not read as entries. +# THE COLUMN IS AN ARGUMENT, because the property is per CHUNK and the sharpest +# statement of that is two columns of ONE row group answering differently. +validity_residual() { # table, column -> page_length - sum(encLen) over its chunks + q "SELECT coalesce(sum(c.page_length - enc), 0) FROM ( + SELECT c.page_length, + (SELECT coalesce(sum( + get_byte(c.encoding_descriptor, 6 + i * 13 + 9) + + get_byte(c.encoding_descriptor, 6 + i * 13 + 10) * 256 + + get_byte(c.encoding_descriptor, 6 + i * 13 + 11) * 65536 + + get_byte(c.encoding_descriptor, 6 + i * 13 + 12) * 16777216), 0) + 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) AS enc + FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = '$1'::regclass + AND c.column_index = $2) c;" | tail -1 +} + +# HOW MANY CHUNKS THE LINE ABOVE ACTUALLY SUMMED. `coalesce(sum(...), 0)` returns +# 0 over an empty set, which is the same 0 the headline arm wants, so a residual +# of 0 is evidence only beside this. A misspelt table, a column index off the end +# or a fixture that never flushed all read as "no bitmap here". +chunks_measured() { # table, column -> row count of the chunks behind the residual + q "SELECT count(*) FROM pgcolumnar.column_chunk c + JOIN pgcolumnar.storage s ON s.storage_id = c.storage_id + WHERE s.relation_oid = '$1'::regclass + AND c.column_index = $2;" | tail -1 +} + +row_groups() { # table -> how many row groups it holds + q "SELECT count(*) FROM pgcolumnar.row_group r + JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id + WHERE s.relation_oid = '$1'::regclass;" | tail -1 +} + +# NO CODEC, deliberately. With one, page_length is the COMPRESSED encoded region +# and the subtraction above stops being the bitmap size -- the arm would then be +# reading a compression ratio and calling it a bitmap. +# +# ALTER DATABASE, NOT SET. Each psql_run is its own session, so a plain SET is +# gone by the next statement and the INSERTs below would run under the default +# zstd. That is not a hypothetical: the first version of this suite used SET and +# measured residuals of 12382 and -10091 where the bitmap is 12500. The negative +# one is what gave it away -- a compressed page can be smaller than the bytes the +# descriptor says it encoded, and no bitmap size can be below zero. +psql_run "ALTER DATABASE $PGC_DB SET pgcolumnar.compression = 'none';" + +# TWO COLUMNS, and the measured one is the SECOND. The key column k exists so the +# fetch arms at the end reach the table: with a single column, an index on it +# answers the query out of the index and the planner takes an Index Only Scan, +# which never calls the table AM's fetch at all. The first version of this suite +# did exactly that -- its fetch arms passed against a build whose fetch path was +# provably broken, and the premise arm is what caught them. +psql_run "CREATE TABLE ve_full (k bigint, v bigint) USING pgcolumnar;" +psql_run "INSERT INTO ve_full SELECT g, g FROM generate_series(1, $ROWS) g;" +psql_run "CREATE TABLE ve_full_h AS SELECT * FROM ve_full;" + +# Every tenth row NULL: enough that the bitmap is genuinely needed, and not so +# many that the column stops holding values to compare. +psql_run "CREATE TABLE ve_nulls (k bigint, v bigint) USING pgcolumnar;" +psql_run "INSERT INTO ve_nulls + SELECT g, CASE WHEN g % 10 = 0 THEN NULL ELSE g END + FROM generate_series(1, $ROWS) g;" +psql_run "CREATE TABLE ve_nulls_h AS SELECT * FROM ve_nulls;" + +full_nulls="$(q "SELECT count(*) FROM ve_full WHERE v IS NULL;" | tail -1)" +some_nulls="$(q "SELECT count(*) FROM ve_nulls WHERE v IS NULL;" | tail -1)" +full_res="$(validity_residual ve_full 1)" +nulls_res="$(validity_residual ve_nulls 1)" +# ONE VARIABLE, USED TWICE. The premise below and the arm it guards must speak +# about the SAME column, and repeating the literal in two calls does not make +# them: @OffgridwithJD measured the third cell -- residual moved to a column that +# does not exist, premise left pointing at column 0 -- and the suite was green +# again over nothing. The twin cannot express that cell at all, because one call +# there returns both the residual and its count; this is the shell saying the +# same thing. +VE_KEY_COL=0 +nulls_key_res="$(validity_residual ve_nulls $VE_KEY_COL)" +nulls_key_chunks="$(chunks_measured ve_nulls $VE_KEY_COL)" +chunks="$(chunks_measured ve_full 1)" +groups="$(row_groups ve_full)$(row_groups ve_nulls)" +codec="$(q "SHOW pgcolumnar.compression;" | tail -1)" +want_bitmap=$(( (ROWS + 7) / 8 )) + +echo "-- ve_full: nulls=$full_nulls residual=$full_res ve_nulls: nulls=$some_nulls residual=$nulls_res key_residual=$nulls_key_res ceil(rows/8)=$want_bitmap codec=$codec groups=$groups" + +# THE PREMISE THE EXACT NUMBER RESTS ON. With a block codec, page_length is the +# COMPRESSED encoded region and the subtraction stops being the bitmap size: the +# arms would read a compression ratio and call it a bitmap. ALTER DATABASE above +# is a statement that has to have taken effect, so it is read back rather than +# assumed -- a misspelt GUC name raises, but a session that did not pick the +# setting up would not. +check "premise: the block codec is off, so the residual is the bitmap and nothing else" \ + "$codec" "none" + +# AND THAT THE INSTRUMENT READ SOMETHING. `coalesce(sum(...), 0)` answers 0 over +# an empty set, which is exactly what the headline arm wants to see, so a +# residual of 0 means "no bitmap" only once this says the sum had chunks in it. +check "premise: the residual was summed over chunks that exist" \ + "$(awk -v n="$chunks" 'BEGIN { print (n + 0 > 0) ? "measured" : "measured-nothing" }')" \ + "measured" + +# AND THAT ceil(ROWS / 8) IS THE WHOLE EXPECTED BITMAP. It is the size of ONE +# group's bitmap; a fixture split across two groups stores two of them, summing +# to the same total only by accident of rounding. One group each, asserted. +check "premise: each fixture is a single row group, which is what the expected size assumes" \ + "$groups" "11" + +check "premise: the first fixture holds no nulls at all" "$full_nulls" "0" + +check "premise: the second fixture holds nulls, so its bitmap is load-bearing" \ + "$(awk -v n="$some_nulls" 'BEGIN { print (n + 0 > 0) ? "has-nulls" : "none" }')" \ + "has-nulls" + +# THE ARM. Exact, not a threshold: with no codec the page is [validity][encoded] +# and the residual IS the bitmap. +check "a column with no nulls stores no validity bitmap" "$full_res" "0" + +# THE SILENT DIRECTION. Dropping the bitmap unconditionally satisfies the arm +# above and loses every null in the table, so this ships beside it. +check "while a column with nulls still stores one, sized one bit per row" \ + "$nulls_res" "$want_bitmap" + +# THE PROPERTY IS PER CHUNK, AND THIS IS THE ONLY ARM THAT SAYS SO. Both arms +# above are satisfied by a writer that decided elision once per ROW GROUP: the +# first fixture's group holds no null anywhere, the second's holds some, and the +# two answers would be the same. ve_nulls has a null-free key column IN THE SAME +# ROW GROUP as its null-bearing one, so only a per-chunk decision can elide one +# and keep the other. +# ITS OWN PREMISE, BESIDE IT RATHER THAN WITH THE OTHERS, so the two move +# together. The arm below expects 0 and 0 is also what the residual returns when +# it summed nothing: a column index off the end of the table answers exactly the +# same as a correctly elided bitmap. Measured by @OffgridwithJD against the first +# version of this suite, which had the premise only on the OTHER residual -- +# pointing this one at column_index 99 left all 24 checks green in both +# harnesses. The residual that protects itself is the one for a column that +# SHOULD store a bitmap, because a bug there reads as a large number; this one +# needs saying. +check "premise: the per-chunk arm's residual was summed over chunks that exist" \ + "$(awk -v n="$nulls_key_chunks" 'BEGIN { print (n + 0 > 0) ? "measured" : "measured-nothing" }')" \ + "measured" + +check "a null-free column elides its bitmap beside a null-bearing one in the same row group" \ + "$nulls_key_res" "0" + +# THE INVARIANT the two arms above exist to protect. A layout change that loses +# or shifts a null is a data-loss bug, not a size regression. +# +# Named for the COLUMN SHAPE rather than the table, so the property is the name +# and the pytest twin can assert it over its own fixture (CONTEXT.md's +# independence rule) instead of inheriting this suite's table names. +for t in full nulls; do + tbl="ve_$t" + check "the $t column reads back exactly what the heap holds" \ + "$(q "SELECT count(*) FROM ( + SELECT k, v FROM $tbl EXCEPT ALL SELECT k, v FROM ${tbl}_h) d;" | tail -1)" \ + "0" + check "the $t column holds no row the heap does not" \ + "$(q "SELECT count(*) FROM ( + SELECT k, v FROM ${tbl}_h EXCEPT ALL SELECT k, v FROM $tbl) d;" | tail -1)" \ + "0" + check "the $t column preserves its null count" \ + "$(q "SELECT count(*) FROM $tbl WHERE v IS NULL;" | tail -1)" \ + "$(q "SELECT count(*) FROM ${tbl}_h WHERE v IS NULL;" | tail -1)" +done + +# THE FETCH PATH IS A SECOND READER, and it is not the one the arms above +# exercise. A sequential scan goes through pgcolumnar_native_load_group; an +# index scan reaches a row through pgcolumnar_fetch_get_row, which reads the +# chunk's bytes itself. Both have to know that the bitmap may be absent, and +# nothing above can tell whether the second one does. +# +# ELISION MAKES THIS PATH MORE LIKELY, not less: the coalesced 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 column this +# change helps most is the one that lands on the per-column path. +# +# THIS IS NOT HYPOTHETICAL. Measured on this tree with the scan path fixed and +# the fetch path not: 33 suites red on the PG17 matrix, and native_index's point +# lookup returned NO ROW for a row that is there -- the chunk's first encoded +# bytes were read as a validity bitmap, so the rows they covered read as absent. +psql_run "CREATE INDEX ve_full_k ON ve_full (k); CREATE INDEX ve_nulls_k ON ve_nulls (k);" + +# enable_custom_scan = off IS THE ONE THAT MATTERS. `enable_seqscan` does not +# govern the columnar custom scan, so without this the planner answers from +# `Custom Scan (PgColumnarScan)` -- a scan, not a fetch -- and every arm below +# becomes a second copy of the scan arms above. Measured: with the three plain +# SETs alone the plan was `Custom Scan (PgColumnarScan) on ve_full`. +FORCE="SET pgcolumnar.enable_custom_scan = off; + SET enable_seqscan = off; SET enable_bitmapscan = off; + SET max_parallel_workers_per_gather = 0;" + +# THE PREMISE THAT MAKES THE THREE ARMS BELOW MEAN ANYTHING, and it is not +# decoration: an INDEX ONLY scan answers out of the index and never calls the +# table AM's fetch, so arms over a single-column table pass against a build +# whose fetch path is broken. "Index Only Scan" does not match this anchored +# pattern, which is the point of anchoring it. +check "premise: the arms below reach the row through an index scan" \ + "$(q "$FORCE EXPLAIN (COSTS OFF) SELECT v FROM ve_full WHERE k = 12345;" | + grep -cE '^[[:space:]]*Index Scan using')" \ + "1" + +# AND THE SAME FOR THE SHAPE THE NEXT TWO ARMS ACTUALLY RUN. The premise above +# plans a point query; the arms below plan a join, which the planner is free to +# serve differently. Asserting one and running the other is how an arm ends up +# licensed by a plan nobody produced. +check "premise: the join arms below reach the columnar side by index too" \ + "$(q "$FORCE EXPLAIN (COSTS OFF) SELECT count(*) FROM ve_full c + JOIN ve_full_h h ON h.k = c.k WHERE c.v IS DISTINCT FROM h.v;" | + grep -cE '^[[:space:]]*(->[[:space:]]*)?Index Scan using ve_full_k')" \ + "1" + +for t in full nulls; do + tbl="ve_$t" + check "the $t column fetched by index matches the heap, row for row" \ + "$(q "$FORCE SELECT count(*) FROM $tbl c JOIN ${tbl}_h h ON h.k = c.k + WHERE c.v IS DISTINCT FROM h.v;" | tail -1)" \ + "0" +done + +# ONE ROW AT A TIME, at positions where a bitmap read out of the wrong bytes +# lands on a neighbour or reports a present row as NULL. The join above is +# satisfied by a fetch that returns the row it was asked for; these are not. +bad="" +for pos in 1 2 3 8 9 4097 $((ROWS / 2)) $((ROWS - 1)) "$ROWS"; do + got="$(q "$FORCE SELECT coalesce(v::text, 'N') FROM ve_full WHERE k = $pos;" | tail -1)" + [ "$got" = "$pos" ] || bad="$bad k=$pos(got=${got:-})" +done +check "single-row fetches through the elided path return the row asked for" \ + "${bad:-same}" "same" + +# THE NULL-BEARING COLUMN NEEDS ITS OWN ONE-ROW PROBE, and a null row in it. +# ve_nulls keeps its bitmap, so these rows exercise the OTHER branch of the same +# per-chunk decision -- and the row that is genuinely NULL is the one a fetch +# that lost the bitmap would answer wrongly in the safe-looking direction. +bad_n="" +for pos in 9 10 11 4096 4097 $((ROWS / 2)) $((ROWS - 1)) "$ROWS"; do + want="N" + [ $(( pos % 10 )) -eq 0 ] || want="$pos" + got="$(q "$FORCE SELECT coalesce(v::text, 'N') FROM ve_nulls WHERE k = $pos;" | tail -1)" + [ "$got" = "$want" ] || bad_n="$bad_n k=$pos(got=${got:-} want=$want)" +done +check "single-row fetches of the null-bearing column return its nulls as nulls" \ + "${bad_n:-same}" "same" + +# --------------------------------------------------------------------------- +# THE TWO GUARDS THE ELISION ADDED, EACH WITH AN ARM OF ITS OWN. +# +# Both live in pgcolumnar_native_load_group and neither is reachable from a +# table this suite writes correctly, so they are exercised by CORRUPTING the +# catalog -- which is what a bit flip or a crafted file does, and what +# corruption.sh does for the fields that predate this change. +# +# ASSERT THE SQLSTATE, NOT THE TEXT. `42501`, `22023`, a login FATAL and a +# missing function all satisfy a grep for "ERROR"; XX001 is ERRCODE_DATA_CORRUPTED +# and comes only from a guard that ran. +sqlstate() { # SQL -> the 5-char SQLSTATE it raises, or NOERR + local out + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -qtA 2>&1 <