docs: say which document to believe when the spec and the code disagree - #500
Conversation
Promised on #492 and owed. That review corrected a line I had taken from the spec (a vector is a fixed 1024 values) which the shipped code does not implement, and the root of it is that the ownership table hands the spec "what the format and the interface ARE" without saying what to do when the code disagrees. Two known instances, both verified rather than recalled: the spec says a vector is a fixed 1024 values; the code sizes it by pgcolumnar.chunk_group_row_limit, default 10000 the spec says the row-group limit is 122880, a multiple of the vector length (section 4, line 77); the code ships stripe_row_limit at 150000 Neither is a defect and neither announces itself. The first is the sharper trap: 1024 is not merely aspirational, it is written into the native storage catalog row as vector_length and then never read back. Checked across src/, sql/ and the extension script, the only occurrences are the column definition, the attribute number, and the INSERT that writes it. A constant that is recorded and never consulted looks exactly like one that is enforced, which is how a reader of either the spec or the catalog reaches a number the scan does not use. So: read the spec for intent and the code for behaviour, and size a fixture from the setting rather than from the spec. That last sentence is the operational part. Someone building a fixture for vector-level skipping in 1024-row units gets one vector per row group at the default, sees Vectors Skipped: 0, and concludes the mechanism is broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approving. This is the right shape for the fix: #492 corrected one line, and one line does not stop the next number being copied. The operational sentence at the end is the part that will actually change behaviour.
The never-read-back claim, verified
I checked it rather than took it, since it is the load-bearing sentence. Every occurrence in the tree:
src/columnar.h:47 #define COLUMNAR_NATIVE_VECTOR_LENGTH 1024
src/columnar.h:223 int vectorLength; (struct field)
src/columnar_write_state.c:529 s.vectorLength = COLUMNAR_NATIVE_VECTOR_LENGTH;
src/columnar_write_state.c:1362 s.vectorLength = COLUMNAR_NATIVE_VECTOR_LENGTH;
src/columnar_metadata.c:1637 values[Anum_native_storage_vector_length - 1] = ...
pgcolumnar--1.0-alpha.sql:171 vector_length integer NOT NULL, -- values per vector (1024)
A definition, two assignments, one write into the tuple, one column declaration. No read site anywhere — not heap_getattr, not a SELECT, nothing. The claim holds exactly as stated.
One thing that makes it sharper than the PR says
The constant is called COLUMNAR_NATIVE_VECTOR_LENGTH, and the native path is the shipped read path. pgcolumnar_native_load_group and pgcolumnar_native_decode_chunk in columnar_reader.c are what a scan actually runs — they are the top of the call tree in a CPU profile of an ordinary query.
So this is not a constant belonging to some future or alternate format that the classic path ignores. It is named for the very code that reads the data, written by that code into that code's own catalog, and then ignored by that code in favour of chunk_group_row_limit. Every signal available to a reader — the spec, the constant's name, the catalog column, and that column's own comment — points at 1024, and the only thing that does not is the behaviour.
Worth a clause, if you want it: "...written into the native storage catalog row as vector_length and then never read back by anything, including the native read path that writes it." Your call; the entry is correct without it.
Sizing a fixture from the setting is the sentence that matters
That is the one I would keep if the rest were cut. Measured, rebuilding the same 200,000 rows at each setting and running the same predicate:
chunk_group_row_limit |
Columnar Vectors Skipped |
|---|---|
| 10000 | 4 |
| 5000 | 8 |
| 1024 | 39 |
Someone sizing in 1024-row units at the default gets one vector per row group, sees Vectors Skipped: 0, and files a bug against a working mechanism.
Small note on scope
The table lists two known divergences. It might be worth saying that two is what has been found, not what exists — the phrasing "in two known places" in the PR body has that hedge and the file's own text does not. A reader who hits a third and finds it unlisted should conclude the list is incomplete rather than that the spec is authoritative there. One word ("known") in the file would carry it.
Neither point blocks; both are optional.
The follow-up I committed to on #492, and owed since that review landed.
#492 corrected a line I had taken straight from the spec (a vector is a fixed 1024 values) that the shipped code does not implement. The root cause is structural rather than a one-line slip: the ownership table hands the spec "what the format and the interface ARE" and then says nothing about what to do when the code disagrees with it. So the next person copies the next number.
Two known instances, both verified against the tree rather than recalled:
pgcolumnar.chunk_group_row_limit, default 10000pgcolumnar.stripe_row_limit, default 150000Neither is a defect, and neither announces itself.
The first is the sharper trap, for a reason worth stating explicitly: 1024 is not merely aspirational. It is written into the native storage catalog row as
vector_length, and then never read back. Checked acrosssrc/,sql/and the extension script, the only occurrences are the column definition (whose own comment says "values per vector (1024)"), the attribute number, and theINSERTthat writes it. A constant that is recorded and never consulted looks exactly like one that is enforced, so both the spec and the catalog point a reader at a number the scan does not use.The operational sentence is the last one: size a fixture from the setting, not from the spec. Someone building a fixture for vector-level skipping in 1024-row units gets one vector per row group at the default, sees
Vectors Skipped: 0, and concludes the mechanism is broken. That is the failure #492 was written to prevent, and it deserved a rule rather than one corrected line.No em or en dashes, matching the file's own convention.
🤖 Generated with Claude Code