From 6814386fbcd1e9573390bc7bdad4303430941d39 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sun, 26 Jul 2026 16:17:36 -0600 Subject: [PATCH] docs: defects get fixed or measured away, not written down and kept Owner's rule: "we don't document bugs and keep them. we document bugs and fix them." Two had accumulated here, and both are now closed. ANALYZE collecting no column statistics was resolved by the July audit as "documented rather than changed" and sat in docs/limitations.md until #159 implemented sampling. That section now describes what the code does, including correlation, which is the statistic that makes vacuum_sorted and Z-order legible to the planner. It also records that reltuples runs a few percent low and that the planner does not use it, rather than leaving a reader to wonder. ColumnarDeleteVectorBufferedDeleted is the other kind of resolution. The audit listed it as retaining the nested-scan shape that #134 fixed next door. I implemented that same last-chunk probe and measured it: 317.6 ms against 299.2 ms without, and doubling the table doubles the time either way, so the term is linear and the extra branch costs more than the walk it skips. The shape is real and the cost is not. Recording the numbers closes it; carrying a patch that buys nothing would not have, so the patch is not here. The rule itself goes in docs/testing.md, next to the differential-oracle and matrix sections, because it is the same class of thing: how this project decides something is done. It says what limitations.md is for (external constraints an extension cannot fix) and what it is not for (defects waiting on someone), and to sweep the docs in the same change as the fix, since ANALYZE read as a limitation for hours after the implementation merged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012uKWWwBDt5TWWS5DR2tzDb --- design/EXTERNAL_AUDIT_2026_07.md | 33 +++++++++++++++++++++++----- docs/limitations.md | 37 ++++++++++++++++---------------- docs/testing.md | 23 ++++++++++++++++++++ 3 files changed, 69 insertions(+), 24 deletions(-) diff --git a/design/EXTERNAL_AUDIT_2026_07.md b/design/EXTERNAL_AUDIT_2026_07.md index fe494a5..e85d228 100644 --- a/design/EXTERNAL_AUDIT_2026_07.md +++ b/design/EXTERNAL_AUDIT_2026_07.md @@ -180,8 +180,31 @@ audit. Nothing from this audit is unmerged. -Two items were left explicitly unfixed, both recorded where they belong rather -than here: `ANALYZE` collects no statistics (#130 documents it; implementing -sampling is the real fix), and `ColumnarDeleteVectorBufferedDeleted` retains the -nested-scan shape that #134 fixed elsewhere, bounded by insert count rather than -table size. +Both items this audit left unfixed have since been fixed, which is the standing +rule here: a defect that is written down and kept is a defect the project has +decided to live with. + +`ANALYZE` collecting no statistics, which #130 recorded as "documented rather +than changed", is implemented in #159: sampling spread across row groups, giving +distribution statistics that match a heap mirror and a correlation of 1 on a +sorted column. `docs/limitations.md` no longer describes it as a limitation. + +`ColumnarDeleteVectorBufferedDeleted`, recorded here as retaining the nested-scan +shape that #134 fixed next door, is closed by measurement rather than by a patch, +which is the other honest way to resolve one of these. + +It does not scale quadratically in practice. One transaction updating rows spread +across many chunk buffers, with a unique index so the probe is on the path at all, +PG18 non-assert: + +| rows updated across many chunk groups | with a last-chunk probe added | as it stands | +| --- | --- | --- | +| 40,000-row table | 317.6 ms | 299.2 ms | +| 80,000-row table | 652.2 ms | 624.1 ms | + +Twice the table costs twice the time either way, so the term is linear, and adding +the same last-chunk probe #134 used makes it marginally slower rather than faster: +the extra branch costs more than the walk it skips, because the chunk list a single +transaction accumulates stays short. The shape is real and the cost is not, so the +right resolution is to record the numbers and stop tracking it, not to carry a +patch that buys nothing. diff --git a/docs/limitations.md b/docs/limitations.md index a1b0ab9..19f3f06 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -36,25 +36,24 @@ only. The rest of the extension runs on any architecture PostgreSQL supports. ## Planner statistics -`ANALYZE` does not collect column statistics for a columnar table. It reports -success and leaves `pg_statistic` empty, so `pg_stats` shows no rows for the -table and autoanalyze has nothing to store either. - -The row count the planner uses is still accurate: it comes from row-group -metadata rather than from `ANALYZE`, so scan and join costs are sized correctly. -What is missing is everything `ANALYZE` would otherwise provide about the values -in a column: most-common values, histograms, distinct counts, null fraction and -average width. Predicates on a columnar table are therefore estimated with the -planner's defaults rather than from the data, which mostly shows up as poor -selectivity estimates for `WHERE` clauses and as join orders chosen from default -distinct counts. - -A table whose plans depend on those estimates is better served by keeping the -filtered columns in a heap table, or by checking `EXPLAIN` output rather than -assuming the planner knows the distribution. - -`TABLESAMPLE` is also unsupported, and unlike `ANALYZE` it says so: it raises an -error rather than returning no rows. +`ANALYZE` collects column statistics for a columnar table: null fraction, +distinct counts, most-common values, histograms and correlation, the same set it +collects for a heap table. Predicates are estimated from the data. + +Correlation is worth calling out because it is the statistic that makes +`pgcolumnar.vacuum_sorted` and Z-order clustering legible to the planner: a table +stored sorted on a key reports a correlation near 1 for that column, and the +planner can then price a range scan over it correctly. + +The row count the planner uses does not come from `ANALYZE` at all. It is derived +from row-group metadata, so it is accurate whether or not the table has been +analyzed. `pg_class.reltuples` runs a few percent low after `ANALYZE`, because +blocks that hold no row-group data (the metapage, and space reserved but not yet +written) count as visited while offering no rows; the planner does not use that +figure for columnar tables. + +`TABLESAMPLE` is unsupported and says so: it raises an error rather than +returning no rows. ## Vacuum and compaction diff --git a/docs/testing.md b/docs/testing.md index 4adcfd5..bb5e367 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -64,6 +64,29 @@ test/native_ios.sh /path/to/pg_config # native index-only scan test/native_projection.sh /path/to/pg_config # native projections ``` +## Defects are fixed, not documented + +A limitation written into the documentation stops looking like a defect. It reads +as a design choice, people plan around it, and nobody reopens it. So a defect is +resolved one of two ways, and writing it down is neither: + +- **Fix it.** Filing an issue is tracking, not resolving; an issue with no change + behind it is a defect the project has decided to keep. +- **Or measure it and show it is not a defect**, then record the numbers so the + next person does not re-litigate it. `design/EXTERNAL_AUDIT_2026_07.md` closes + the `ColumnarDeleteVectorBufferedDeleted` nested scan this way: the shape is + real, the cost measured linear rather than quadratic, and adding the obvious + cache made it slower. + +`docs/limitations.md` is for what is genuinely out of scope or blocked by an +external constraint: an extension cannot change WAL behaviour, PostgreSQL 13 and +14 lack an API. It is not a parking space. Anything in it that is unfixed only +because nobody has fixed it does not belong there. + +When a fix lands, sweep the docs in the same change. `ANALYZE` collecting no +statistics sat in `limitations.md` as a limitation after the implementation had +already merged, which is worse than either state alone. + ## Differential oracle `test/differential.sh`, `recovery`, `fuzz`, `hardening`, and `concurrent_diff`