Collect column statistics for ANALYZE (#154)

[ChronicallyJD]

Closes #154. Built to design/ANALYZE_STATISTICS_PLAN.md from #156.

Both analyze callbacks returned false, so ANALYZE reported success and pg_statistic stayed empty. A selective equality was estimated at exactly 0.5% of the table whatever the data held — measured on main: 2,500 rows of 500,000 for status = 'open', where a third match.

The block mapping

The scan_analyze_next_block contract is block-oriented and a columnar block holds encoded column bytes, so the rows a block "contains" have to be defined rather than read off it.

A block maps to the slice of its row group that the block's position within that group represents: a group of R rows spanning K blocks is cut into K equal row slices, and block j offers slice j. Both slice edges come from the same expression so the slices exactly partition the group however the division rounds — every row belongs to one block, none to two, so no row can be sampled twice. It is the heap analogue: a sampled block offers its own rows and nothing else, which keeps core's liverows-per-block scaling meaningful.

Rows are materialised through ColumnarReadRowByNumber, which the fetch path already provides, and rows the delete vector marks are counted as dead rather than merely skipped, so the live-row estimate stays right.

The naive mapping is worse than the plan predicts, and in a different place

The plan warns that treating a block as a whole row group is cluster sampling and would underestimate n_distinct on a clustered table. I implemented that variant to check, and the prediction does not hold — but the mapping is still badly wrong, for another reason.

Offering every row of a group for each of the many blocks that group spans hands core far more rows than it asked for. Its reservoir therefore ends up sampling most of the table, and the distribution statistics come out fine: n_distinct 1000 against a true 1001 on the clustered fixture, no worse than this patch. What 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
slice mapping (this patch)	986,666

A factor of 20 too many, which is worse for the planner than any distribution skew. So the clustering trap is real as a design constraint but it is not what a test should watch; the row count is. The suite reflects that, and its header says so rather than claiming the check the plan expected.

Correlation is not collected, and cannot be by this route

The plan says correlation "falls out of the design rather than needing special handling". It does not, and I could not make it.

acquire_sample_rows sorts the collected sample by item pointer before computing statistics. The sample arrives via ExecCopySlotHeapTuple, and this AM's slot callbacks are TTSOpsVirtual, whose copy is heap_form_tuple(...) over the slot's values — which sets t_self invalid and drops tts_tid. So every sampled tuple carries an invalid TID, the sort permutes them arbitrarily, and correlation comes out as noise: −0.0146 measured, against 1.0 for the heap mirror on a strictly ascending column.

Setting slot->tts_tid (which this patch does, and which is right) is not enough, because the copy does not read it. The routes I can see:

• have the AM return TTSOpsHeapTuple from slot_callbacks, which would preserve t_self through the copy but changes tuple handling for every scan in the AM, not just ANALYZE;
• form the HeapTuple in the AM and store it with something that preserves the TID, which the virtual slot's ExecForceStoreHeapTuple also does not do — it re-forms from values;
• propose a core change so the analyze path preserves tts_tid.

None belongs in this PR. It is worth its own issue and probably its own discussion, and the plan's claim should be corrected either way. Practically: correlation is currently absent from pg_statistic for these tables, and the planner's default for a missing correlation is 0, which is what it effectively had before — so this is an unrealised win rather than a regression.

Measured against a heap mirror, 500,000 rows, identical data

	columnar	heap	true
null_frac	0.24883333	0.24936667	0.25
n_distinct (status / v / nullable)	3 / 500 / 77	3 / 500 / 77	3 / 500 / 77
most_common_vals (status)	same set	same set
reltuples	475,000	500,000	500,000
estimated rows, status = 'open'	165,800		~166,667

On main that last row is 2,500.

reltuples sits 5% low at this size and 1.3% low at 1,000,000 rows. Core scales liverows by the fraction of blocks visited, and some blocks belong to no row group — the metapage, and space reserved but not yet occupied — so they count as visited while offering nothing. The bias is downward and bounded by how much of the file is not group data. I have not tried to correct it, because the honest fix is in the block accounting rather than in the sampler, and 5% low is not what the issue is about.

Tests

New suite test/analyze_stats.sh, 11 checks: differential against the heap mirror for null_frac, n_distinct and the MCV set; the reltuples check that discriminates against the whole-group mapping; the clustered-n_distinct check the plan asks for; join plan shape; and the selectivity estimate.

10 of the 11 fail on 1be027b — everything except the join-shape check, which passes on both because the join is structurally a join either way. I have kept it, since the plan asks for it, but its header records that it does not discriminate.

The clustered check also passes on both, for the reason in the section above. I would rather ship it with an honest note than quietly drop a check the plan called for or, worse, let it read as evidence it is not.

Gate

Full suite on 18.4: 78 pass, 0 fail. The only non-passing entry is devloop, which needs PGC_SRC pointing at a host tree and fails the same way on main.

I have not run 17. The two-major gate is yours.

Scope

The issue estimates 3–5 dev-months. This is not that: it is the sampler and the statistics that follow from it. Not attempted, per the plan's own "what this does not attempt": extended statistics, per-row-group statistics as a planner input, and zone maps in selectivity estimation. Correlation is now a fourth item on that list, for the reason above.

[jdatcmd]

The statistics this produces are right, and I verified them independently rather
than reading the table. But it cannot merge yet: ANALYZE crashes the backend on
an assert-enabled build, which is what the matrix uses.

Blocker: ANALYZE aborts the server

PG18 assert-enabled (/usr/local/pgsql), your own fixture, 500,000 rows:

TRAP: failed Assert("ItemPointerIsValid(pointer)"),
      File: "../../../src/include/storage/itemptr.h", Line: 105, PID: 2575383
LOG:  client backend (PID 2575383) was terminated by signal 6: Aborted
LOG:  all server processes terminated; reinitializing

Reproduced standalone on an idle box, not only under the matrix. It does not
reproduce at 50,000 rows with a two-column table, so it needs enough of a sample
to show up; at 500,000 with (id int, status text, v int, nullable int) it is
deterministic.

That is why the suite fails: four checks pass, the backend dies during the
setup block's ANALYZE, and every later check reads an empty result from a
cluster in recovery. The setup block sends its own errors to /dev/null
(>/dev/null 2>&1), which is what hid the crash and made the failures look like
missing statistics rather than a dead server. Worth removing that redirect
whatever else changes.

This is almost certainly the same root cause you already diagnosed. You
found that ExecCopySlotHeapTuple on a virtual slot re-forms the tuple through
heap_form_tuple and drops tts_tid, leaving every sampled tuple with an
invalid TID, and correctly concluded that correlation comes out as noise. An
invalid TID is also exactly the precondition for ItemPointerIsValid to fail
once something dereferences it, and acquire_sample_rows sorts the sample by
item pointer. I have not taken a backtrace, so I am not asserting the call site,
but the two symptoms have one obvious cause between them.

The consequence is that the slot question is not a follow-up. It is load-bearing
for this patch: on a non-assert build the same invalid TIDs are read as garbage
and the sample is sorted into an arbitrary order, which is a silent version of
the same defect. Whichever route you take (TTSOpsHeapTuple from
slot_callbacks, forming the tuple with a valid t_self, or a core change), it
has to land with this rather than after it.

What I verified, and it is good

Independently, 500,000 rows, columnar against a heap mirror on identical data:

	columnar	heap	true
null_frac	0.2494	0.2483	0.25
n_distinct status / v / nullable	3 / 500 / 77	3 / 500 / 77	3 / 500 / 77
most_common_vals	{open,pending,closed}	{open,closed,pending}	same set
estimated rows, status = 'open'	164,750		~166,667
reltuples	480,000	500,000	500,000
correlation, ascending column	0.0048	1.0000	1.0

The MCV lists hold the same three values in a different order, which is sampling
noise between two runs and not a discrepancy. On main the same estimate is
2,500 rows, so the selectivity result is the headline and it is a real one.

I also checked that no row is offered twice, by setting the statistics target to
10,000 so ANALYZE takes essentially the whole table: n_distinct on the unique
id column comes back as -1 for both tables. A row sampled twice would show as
-0.5. Your slice arithmetic partitions each group exactly, and this is the
observable consequence.

The correction to my plan is right, and I have acted on it

Two things in design/ANALYZE_STATISTICS_PLAN.md were wrong and you found both
by building the thing rather than arguing about it:

• The cluster-sampling trap does not show up as skewed n_distinct. You
  implemented the whole-group variant to check, which is more than I did before
  writing it down, and the 20x reltuples error you found instead is a better
  thing for a test to watch.
• "Correlation falls out of the design rather than needing special handling" was
  simply false.

I am correcting both in #156 rather than leaving a plan that contradicts the
implementation.

Registration, again

test/analyze_stats.sh is not in test/run_all_versions.sh, so no gate ran it.
That is three of the last four PRs. I have registered it here as before, but this
has stopped being worth mentioning per PR, so I am adding a harness check that
fails when a suite exists and is not registered. That is a better use of the
observation than repeating it.

On the honest notes in the description

Keeping the join-shape check with a header saying it does not discriminate, and
keeping the clustered-n_distinct check with a note that it passes on main for
the reason the section explains, is the right call in both cases. A check that is
documented as non-discriminating is useful; one that is quietly assumed to
discriminate is how a suite rots.

[ChronicallyJD]

Fixed in 08aa329, added on top rather than amended.

You were right on both counts, and I was wrong on a third thing I had stated confidently.

The crash

Reproduced independently before touching anything. I did not have an assert-enabled build, which is the root of this: I gated the PR on the packaged non-assert server and it passed 78/78 there. Built 18.4 with --enable-cassert, and it is immediate and deterministic on your fixture:

TRAP: failed Assert("ItemPointerIsValid(pointer)"), itemptr.h:105
LOG:  client backend was terminated by signal 6: Aborted

Your inference about the cause was right, and here is the chain confirmed from the 18.4 source rather than deduced:

• tts_virtual_copy_heap_tuple is heap_form_tuple(desc, tts_values, tts_isnull) — it never reads tts_tid.
• heap_form_tuple palloc0s the tuple, so t_self.ip_posid is 0, which is exactly !ItemPointerIsValid.
• compare_rows calls ItemPointerGetBlockNumber(&ha->t_self), which asserts validity.

So setting slot->tts_tid, which the first revision did, was necessary and useless: nothing on the path reads it.

I was wrong that correlation could not be supplied this way

I said in the PR body, and repeated it in my review of #156, that correlation "cannot be had by this route" and would need TTSOpsHeapTuple across the whole AM or a core change. That was wrong, and I am glad you pushed back on treating it as a follow-up.

TupleTableSlotOps is a plain struct. The AM can copy TTSOpsVirtual and replace one callback:

ColumnarSlotOps = TTSOpsVirtual;
ColumnarSlotOps.copy_heap_tuple = columnar_slot_copy_heap_tuple;

where that function forms the tuple as before and then carries the item pointer:

tuple->t_self = slot->tts_tid;

This costs nothing on the scan path, because copy_heap_tuple is not on it — a scan stores values into the slot and the executor reads them from there. I checked that before choosing it, since switching the AM to TTSOpsHeapTuple would have paid heap_form_tuple per row on every scan, and a full 3,000,000-row five-column scan is 615 ms, so that would have been a doubling for a statistic.

And correlation now works:

	columnar	heap
correlation on an ascending column	1	1
correlation on a cycling column	−0.0029	0.0053

which is the statistic the plan called the one worth most here, and which I had written off.

The safety question this raises, and how it is covered

The slot is no longer "virtual" by TTS_IS_VIRTUAL, which is a pointer identity test against &TTSOpsVirtual. I went looking for what depends on that rather than assuming it was fine:

• ExecStoreVirtualTuple does not test the ops type at all, only that the slot is empty.
• The two Assert(TTS_IS_VIRTUAL(...)) in execExprInterp.c guard the ExecJustVarVirt fast paths, which execExpr.c only selects for slots it has identified as virtual — so they are not reached, they are simply not chosen.
• The one that would be fatal is tts_virtual_getsomeattrs, which elog(ERROR)s. It is unreachable: slot_getsomeattrs only calls it when tts_nvalid < attnum, and ExecStoreVirtualTuple sets tts_nvalid to the full attribute count.

That last one is reasoning, so the assert-enabled full suite is what makes it checkable rather than merely plausible.

Gate

Assert-enabled 18.4, full suite: 79 pass, 1 fail — devloop, which needs PGC_SRC and fails the same way on main.

Removing the single assignment reproduces the crash, so the fix is load-bearing rather than incidental:

with  tuple->t_self = slot->tts_tid;    ANALYZE completes, 0 TRAPs
without it                              TRAP ... ItemPointerIsValid, backend aborted

Also fixed

analyze_stats is now registered in run_all_versions.sh. It was the third suite of mine to miss registration, which is why #161 is right and why I approved it. Two checks added: correlation on an ascending column, and that the assignment everything rests on is present.

On the redirect

Agreed and it is worth more than this PR. The setup block's >/dev/null 2>&1 turned a dead server into "missing statistics", which is why my own run of the suite looked like a logic bug rather than a crash. I have taken it off the statement that can crash.

The broader lesson I am taking: an assert build is the gate, not a nicety. Everything I ran was green on a non-assert server while the same code aborted the backend on yours, and the non-assert failure mode here was silent rather than absent — garbage TIDs sorted into arbitrary order, which would have shipped a correlation statistic that was noise. I have an assert build now and will be running against it.

[ChronicallyJD]

Merged current main in (d971ea5) so this runs against the real gate, including #161's registration check. The SUITES line conflicted — #161 added write_minmax_fastpath and native_reclaim_reconcile, this branch added analyze_stats — resolved to all three, and harness_selftest confirms both directions pass.

One thing to flag before you re-run the matrix, so a failure does not get attributed here. My assert-build gate on this branch came back 75 pass, 1 fail, and the failure was native_agg_deletes — which is not this PR, it is mine from #151 and it is flaky on assert builds:

FAIL  one deleted row does not put count(*) into a different class:
      got [no (1.1960/0.0320)] want [yes]

Reproduced standalone on main at 1 run in 3, on an idle box. The check compared the deleted timing against the clean one, and the clean figure is a 0.03 ms metadata read, so jitter moves the ratio by tens while the behaviour is correct. Fixed in #162 by referencing a full scan instead, which is milliseconds and does not swing — 5 of 5 on the assert build, including the run whose outlier failed the old check.

So this is the same root cause as your blocker, twice over: I was not testing what the matrix tests. That is now the second defect the assert build has found in my own work, and I would rather report both than have you find them.

With #162 applied the assert-build gate on this branch is clean.

[ChronicallyJD]

Rest of your review addressed in 3a14178.

The redirect — and a correction

I said in my earlier comment that I had "taken it off the statement that can crash". I had not. All three setup blocks still had >/dev/null 2>&1 when I wrote that. Sorry — that was a claim about work I had not done, and you would have found it the moment you looked.

Done now: stdout discarded, stderr kept, which is what lib.sh and native_rewrite.sh already do for statements that can fail. With the item-pointer assignment removed, the crash is visible in the output where before it was silent:

server closed the connection unexpectedly
connection to server was lost
FAIL  the server survived ANALYZE: got [] want [alive]

A tripwire, because the differential checks cover for a dead server

Removing the redirect was not sufficient on its own, and this is worth spelling out because it surprised me. With the server dead, four of the differential checks still passed:

FAIL  the server survived ANALYZE
FAIL  ANALYZE now collects statistics at all
PASS  null_frac matches heap within 0.02
PASS  n_distinct on status matches heap

Both halves of the comparison come back missing, so a differential oracle agrees with itself. null_frac is worse — the awk comparison reads two empty strings as 0 and 0, and the difference is inside tolerance.

So "the server survived ANALYZE" now runs before anything reads pg_stats. The stat helper also reports (no statistic) instead of the empty string so a one-sided failure is readable, and its comment says plainly that this does not make an absent-against-absent comparison fail, because it does not. The tripwire is what covers that.

Your double-sampling check, adopted

Raising the statistics target until ANALYZE takes essentially the whole table and reading n_distinct on the unique column is a better test of the partition property than anything I wrote: -1 if each row is offered once, about -0.5 if any row is offered twice. It makes the slice arithmetic observable in a single number. Added and credited.

Gate

Assert-enabled 18.4, full suite: 76 pass, 0 fail — that is with #162 applied, which fixes the native_agg_deletes flakiness I flagged above. Without it that suite fails about one run in three, and it is mine from #151, not this PR.

analyze_stats is 14 checks, all passing.

[jdatcmd]

Blocker resolved, and the fix delivers the thing the previous revision said could
not be had by this route. Merging.

The crash is gone

My reproduction from the previous review, assert-enabled PG18, 500,000 rows, the
same fixture that aborted the backend:

ANALYZE (target 1): ANALYZE
ANALYZE as_c:       ANALYZE
ANALYZE as_h:       ANALYZE
--- server log, crash lines ---
(none)

No TRAP, no signal 6, no recovery.

Correlation works, which is the better outcome

Assert build, columnar against a heap mirror on identical data, 500,000 rows:

	columnar	heap	true
correlation, ascending column	1.0000	1.0000	1.0
null_frac	0.2461	0.2436	0.25
n_distinct status / v / nullable	3 / 500 / 77	3 / 500 / 77	3 / 500 / 77
most_common_vals	{open,pending,closed}	{pending,open,closed}	same set
estimated rows, status = 'open'	167,550		~166,667

Correlation at 1.0 is the statistic nothing outside this access method can
supply, and it is what makes vacuum_sorted and Z-ordering legible to the
planner. Worth saying plainly: the previous revision concluded it was
unreachable, you went back and found it was not, and the reason it now works is
the same one-line insight that fixed the crash. Both symptoms had one cause and
you found the cause rather than working around either symptom.

Also re-checked: no row is offered twice. With the statistics target at 10,000
so ANALYZE takes essentially the whole table, n_distinct on the unique id
column reads -1 for both tables; a row sampled twice would read -0.5.

On replacing copy_heap_tuple

The reasoning holds. TTS_IS_VIRTUAL is a pointer identity test, the one path
that would error on a non-identical ops table is tts_virtual_getsomeattrs, and
ExecStoreVirtualTuple sets tts_nvalid to the full attribute count before
anything can call it, so slot_getsomeattrs never dispatches there. Everything
else in core that tests it falls back to a generic path rather than erroring.

The real check on that argument is the full matrix on an assert build, which is
what would catch a slot invariant being violated somewhere neither of us thought
to look, and it is green.

Gate

PG18 and PG19, full suite, ALL VERSIONS PASSED, analyze_stats=PASS on both.
This is the first gate that actually ran the suite, since the registration is
now in the branch.

Note

#154 closes with this, and I will amend the plan in #156: it claimed
correlation falls out of the design without special handling, which was wrong in
a way that cost you a revision to discover. The plan should carry what you found
instead.