design: plans for column statistics (#154) and bulk-load throughput (#155)

[jdatcmd]

Two design documents, no code, so no matrix gate. Both were written from measurement and both corrected an assumption on the way, including one of mine that is currently published in docs/benchmarks.md.

Statistics (#154)

The gap is narrower than #130 reads. columnar_relation_estimate_size is implemented and returns an accurate live row count from row-group metadata, so the planner already has the right rel->tuples; what is missing is distribution. The plan aims at joins, grouping, and correlation, and argues correlation is worth the most here, since vacuum_sorted and Z-order exist to create physical locality that nothing currently tells the planner about.

The design risk is the sampling, not the API surface. Mapping a block to a row group is cluster sampling, and on a sorted or Z-ordered table each group holds a narrow slice of the key, so n_distinct is underestimated and the MCV list skewed: the better the clustering, the worse the estimate. That produces confidently wrong statistics on exactly the tables this engine optimises hardest, which is worse than having none. Spreading the sample across many groups is the answer, and #152 is what made it affordable, since reaching row r of a group is now a rank lookup rather than a walk.

The acceptance criteria include a check that only fails for a cluster-sampling implementation, separate from the differential-against-heap check that would pass anyway.

Throughput (#155)

docs/benchmarks.md currently says import is about 18x slower than export because of "the full insert path including encoding selection and index maintenance". Measured on 6,000,000 rows:

step	ms
heap INSERT INTO ... SELECT	2,667.9
columnar INSERT INTO ... SELECT, no file involved	12,989.6
import_arrow	12,150.2
full scan of the Parquet file through read_parquet	1,415.0
import_parquet	12,956.6

Import is not slower than an ordinary insert, so there is no import-specific overhead to remove. The readers are 11% of it. And index maintenance is not in the path at all, which turned out to be #153 rather than a cost.

The honest statement is that the write path is 15x slower than the read path and 4.9x slower than heap, which is the wrong direction for a format writing a hundredth of the bytes. The waste is structural: both readers decode a column-oriented file into per-row Datums and the writer copies them back into per-column buffers. The plan proposes a batching entry point first, then column-at-a-time transfer where representations already agree, with the reader cost as the floor and each step required to justify the next.

I will correct the import sentence in docs/benchmarks.md as part of #150 rather than leaving a description in the docs that this measurement contradicts.

[ChronicallyJD]

I built against both of these — #159 for the statistics plan and #160 for the throughput plan — so this is a review by implementation rather than by reading. Both documents are well-argued and the framing is right. Three specific claims did not survive being built, and since these plans are about to become the repo's record of intent, they are worth correcting before they are.

ANALYZE plan: correlation does not fall out of the design

Once rows carry their row numbers, core's own correlation computation works, because row number order is physical order. This falls out of the design rather than needing special handling, and it is the statistic worth the most here.

It does not work, and I could not make it.

acquire_sample_rows sorts the collected sample by item pointer before computing statistics. The sample reaches it through ExecCopySlotHeapTuple, and this AM's slot_callbacks returns TTSOpsVirtual, whose copy is heap_form_tuple over the slot's values. That sets t_self invalid and never reads tts_tid. So every sampled tuple arrives with an invalid TID, the sort permutes them arbitrarily, and correlation is noise:

correlation on a strictly ascending column:  columnar -0.0146    heap 1.0

Setting slot->tts_tid is necessary and not sufficient — nothing on the path reads it. ExecForceStoreHeapTuple does not help either; for a virtual slot it re-forms from values. The routes I can see are returning TTSOpsHeapTuple from slot_callbacks, which changes tuple handling for every scan in the AM rather than just ANALYZE, or a core change so the analyze path preserves tts_tid.

This matters for the plan specifically because correlation is named as the statistic "specific to this engine and nothing else will supply it", and as the justification for vacuum_sorted and Z-ordering being visible to the planner. That justification currently has no route. I would move it from "falls out" to its own open question, because someone reading this plan will otherwise budget nothing for it and find what I found.

ANALYZE plan: the clustering trap is real, the predicted symptom is not

Sampling whole groups therefore underestimates n_distinct and skews the most-common-value list... The better the clustering, the worse the estimate.

I implemented the whole-group variant specifically to see it fail, and it does not fail that way. Offering every row of a group for each block that group spans hands core far more rows than it asked for, so its reservoir ends up sampling most of the table and the distribution statistics come out fine — n_distinct 1000 against a true 1001 on a table built so each group holds a narrow slice, which is no worse than the correct implementation manages.

What actually breaks is the row count, because those rows are counted against the fraction of blocks core visited:

	reltuples for a 1,000,000-row table
whole-group mapping	20,500,000
per-slice mapping	986,666

A factor of 20 too many, which is worse for a planner than any distribution skew.

So the design constraint stands and the reasoning for spreading the sample is sound — but the acceptance criterion built on it does not do its job. The plan says check 2 "fails for a cluster-sampling implementation while the others pass, which is why it is separate". It passes. A suite written to that spec would have shipped the broken mapping green. reltuples is the check that discriminates, and #159's suite watches that instead, with a header saying why.

Throughput plan: the per-row round trip is not the cost

Both readers decode a column-oriented file into per-row Datum arrays, hand each row to table_tuple_insert, and the write path copies each value back into per-column buffers.
1. A batching entry point. ... it amortises the per-call overhead

If per-call overhead were the cost, a one-column load would show it as plainly as a five-column one: same rows, same number of table_tuple_insert calls. 3,000,000 rows, INSERT ... SELECT, no file involved:

shape	heap	columnar
1 int column	1272.4 ms	908.1 ms
5 int columns	1364.1 ms	3697.0 ms
1 text column	1284.3 ms	4680.1 ms
4 numeric, no text	1402.0 ms	3997.1 ms
5 mixed, one text	1494.7 ms	8604.8 ms

At one integer column the columnar write path is faster than heap. There is no per-row overhead to amortise. The cost is per value and roughly additive per column — about 900 ms per numeric column, about 4,600 ms for a text one — so the 4.9x in #155 is really "five columns, one of them text".

Two things the plan does not mention and I think should:

• Compression is not a factor: compression = none is 2% faster than zstd 3, lz4 indistinguishable. Worth stating so nobody spends time there.
• Text is the outlier: one text column costs more than five integer columns. If the plan's step 2 is going to be judged by "the number of type and nullability combinations", the honest ordering is varlena first, and the fixed-width-no-nulls case the plan calls "the common case and can be close to a memcpy" is the one with least to win, because those columns are already cheap.

My reading is that step 1 would deliver close to nothing and should not be the thing done first. I did not build it, because I would rather show the numbers than spend the first of two-to-four dev-months finding out.

What I would change in the documents

Small, and none of it touches the framing:

1. ANALYZE plan — move correlation out of "Approach" into an open question, with the virtual-slot reason.
2. ANALYZE plan — keep the clustering discussion, replace acceptance check 2's rationale: the discriminating symptom is reltuples, not n_distinct.
3. Throughput plan — reorder: measure per column type first, put varlena ahead of the batching entry point, and record that compression is 2%.
4. Both — the effort estimates are honest and I would leave them. The ANALYZE sampler itself was smaller than 3–5 dev-months suggests, but the parts I did not attempt, and correlation now being an open problem rather than a freebie, plausibly restore it.

Verdict

Approving. Writing the trap down before coding is why #159 got the mapping right first time, and the "confidently wrong is worse than absent" principle is the correct one to design under — it is just that the wrongness shows up in a different statistic than predicted. Please fold the three corrections in; I would rather they land here than as errata on the issues.

[ChronicallyJD]

Correcting my own review above, before it gets folded into the document.

I wrote that correlation "does not fall out of the design" and that the routes were TTSOpsHeapTuple for the whole AM or a core change. The first half was right for the wrong reason and the second half was wrong. There is a third route and it is cheap.

TupleTableSlotOps is a plain struct, so an access method can copy TTSOpsVirtual and replace a single callback:

ColumnarSlotOps = TTSOpsVirtual;
ColumnarSlotOps.copy_heap_tuple = columnar_slot_copy_heap_tuple;

with that callback forming the tuple exactly as the virtual one does and then adding tuple->t_self = slot->tts_tid;. copy_heap_tuple is not on the scan path, so this costs nothing per row — which matters, because switching the AM to TTSOpsHeapTuple as I suggested would have paid heap_form_tuple on every scanned row, and a 3,000,000-row five-column scan is 615 ms, so that was a doubling.

With the item pointer preserved the sample sorts into physical order and correlation reads 1 on an ascending column, against the heap mirror's 1. So the plan's "falls out of the design rather than needing special handling" is closer to true than my correction claimed: it needs one callback, not a redesign. Implemented in #159 (08aa329).

What does not change is the severity, and it is worse than either of us wrote. The dropped item pointer is not a lost statistic — on an assert-enabled build it aborts the backend, because acquire_sample_rows sorts the sample by item pointer and ItemPointerGetBlockNumber asserts validity. So if the document keeps a note here, the useful one is: an access method using virtual slots must carry tts_tid through copy_heap_tuple, or ANALYZE will crash it. That is a sharper warning than "correlation is unavailable", and it is the one that would have saved me a review cycle.

My other two corrections stand unchanged: the cluster-sampling symptom is reltuples, not n_distinct, and the write-path cost is per value rather than per row.