docs: record the analyze and domain-pruning changes (#414, #483, #485) - #497
Conversation
Three merges today changed behaviour a user can observe and none of them carried a CHANGELOG entry. The one that most needed writing down is under Changed rather than Fixed, and it is the reason this commit exists: #414's histogram work ALTERS AN EMITTED VALUE. Length and both endpoints are unchanged, so #414's exactness claim about the minimum and maximum holds, but an interior bound can move by one position. Both forms are valid equi-depth histograms and core's is the one the planner's estimators were tuned against, so the change is right; it is also invisible unless somebody diffs pg_stats across the upgrade and is told to expect it. Saying "intended, not a regression" in the entry is the whole point of having one. #485 goes under Fixed with the part that is easy to miss: the wrong number matters less than the fact that null_frac and the most-common frequencies were normalised against DIFFERENT populations, so null_frac + sum(freqs) + rest = 1 stopped holding and eqsel subtracts both. The entry also records what the fix gives up, since "null_frac is a metadata read" was a stated selling point of #414 slice 1 and is now gone. #483 goes under Fixed beside #477, which is the same defect one type-resolution away, so a reader who hits one finds the other. It states explicitly that answers were never wrong and the cost was reading the whole table, because a pruning entry with no such sentence invites a correctness scare. Not recorded: #486 and #487, which changed only the test harness. The file covers notable changes to pgColumnar, and a suite that stopped reporting a match as absent is not one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approving. Every claim I could check independently holds, and the two editorial decisions are the right ones.
Checked rather than taken
"an interior bound can move by one position" — this is exact, not approximate, and it is worth knowing it is exact. With x = i*nvals/num_hist, percentile_disc resolves to ceil(x) - 1 and the stride to floor(x - i/num_hist). Where x is not an integer those are floor(x) and either floor(x) or floor(x) - 1; where x is an integer they coincide. So the difference is 0 or 1, never more, for any cardinality. The sentence is safe to leave unqualified.
"both endpoints are the same" — at i = 0 both resolve to index 0; at i = num_hist both resolve to nvals - 1. So #414's exactness claim about the true minimum and maximum survives the change, which is the thing a reader would most want reassurance on.
The #485 numbers — 0.100000 against a true 0.143062, and 1,200-implied beside 900-implied with 900 present, both match what I measured.
"ordering and hashing are unchanged" for #483 — the writer takes its comparison and hash from lookup_type_cache(att->atttypid) at columnar_write_state.c:282 and :1715, the reader at columnar_reader.c:443. Same expression on both sides, and #494 deliberately left it alone.
The two editorial calls
Recording what #485 gives up is the part I would defend if anyone questions the length. "null_frac is a metadata read" was a stated property of #414 slice 1 and it is gone; a changelog that lists only gains is a marketing document. The entry also correctly says a metadata-only fast path would need a live-row count, which is the same read — so it is not a regression waiting to be reclaimed.
Placing #483 beside #477 is right for the reason given, and there is now a third: #493 is the same disagreement a third time, between the scalar node and the two vectorized aggregate nodes. If you wanted one more sentence anywhere, it would be a pointer from these two to #493, so a reader who arrives at the pair learns the class is still open rather than closed by these fixes.
Omitting #486 and #487 is correct and saying so in the PR description is what stops someone re-adding them later.
One consequence this documents but does not fix
The COMMENT ON FUNCTION pgcolumnar.analyze(...) on main still reads:
taking null_frac exactly from the zone maps rather than sampling (#414)
Which is what this CHANGELOG entry says is no longer true. That is my own omission from #488 — I changed the function body and the suite and left the comment claiming the old mechanism. It is user-visible via \df+.
Opened as #498, one line, so it does not complicate this PR. Take it or fold it in, whichever you prefer.
Three merges today changed behaviour a user can observe, and none carried a CHANGELOG entry: #488 (both halves) and #494.
The one that most needed writing down
Under Changed, because #414's histogram work alters an emitted value the planner consumes:
The change is right. It is also completely invisible until somebody diffs
pg_statsacross an upgrade, at which point "intended, not a regression" is the sentence that saves them an afternoon. That is the whole reason for the entry.Under Fixed
#485, with the part that is easy to miss placed above the numbers: the wrong value matters less than the fact that
null_fracand the most-common frequencies were normalised against different populations, sonull_frac + sum(most_common_freqs) + rest = 1stopped holding andeqselsubtracts both. The entry also records what the fix gives up — "null_frac is a metadata read" was a stated selling point of #414 slice 1 and is now gone — because a changelog that only lists gains is not a record.#483, placed directly beside #477, since they are the same defect one type-resolution apart and a reader who hits one should find the other. It says explicitly that answers were never wrong and the cost was reading the whole table, because a pruning entry without that sentence invites a correctness scare it does not deserve.
Not recorded, deliberately
#486 and #487 changed only the test harness. This file covers notable changes to pgColumnar, and a suite that stopped reporting a match as absent is not one. Recorded here so the omission reads as a decision rather than an oversight.
Verification
docs_style.shpasses, including its CHANGELOG-specific check that the file carries no em or en dash.🤖 Generated with Claude Code