feat: most-common values, exact and excluded from the histogram (#414 slice 3b)

[jdatcmd]

Closes the slice 3b half of #414: most_common_vals and most_common_freqs, the
histogram exclusion they force, and the differential harness you asked to land
with them rather than after.

The selection rule was not a design decision

I went to analyze.c rather than reasoning from sampled behaviour, and core
answers it outright. analyze_mcv_list() opens with:

/*
 * If the entire table was sampled, keep the whole list.  This also
 * protects us against division by zero in the code below.
 */
if (samplerows == totalrows || totalrows <= 1.0)
    return num_mcv;                                    /* analyze.c:2995 */

The whole significance filter -- a continuity-corrected Wald interval over a
hypergeometric variance -- exists to judge whether a sample frequency is
trustworthy enough to store. We read the entire column, so core's own answer is
"keep them". What is left is mechanical, and all of it read off the same file:

rule	core	what it means here
only values with count > 1 are eligible	:2549	a unique column gets no list, matching nmultiple == 0 at :2588
top attstattarget by count	:2552-2564	ORDER BY count DESC LIMIT target
significance filter	:2995	skipped, per the quote above
frequency over TOTAL rows, nulls included	:2720	count / totalrows, not over non-null rows

The denominator is the one that fails quietly, so the fixture catches it

Dividing by the non-null count instead scales every frequency by
1/(1-null_frac). On the new cat column, which is deliberately one-tenth null:

value	true frequency	what the wrong denominator gives
7	0.18	0.2
42	0.108	0.12
99	0.054	0.06

Three numbers that are individually plausible, correctly ordered, and sum to
less than one. Nothing raises. Injected, it fails three checks with exactly
those values.

The exclusion changes slice 3 rather than only adding to it

Core builds the histogram from what is left after the most-common values are
collapsed out (:2768-2799) and sizes it from ndistinct - num_mcv (:2744).
Keeping them counts those values twice in selectivity: eqsel takes the
frequency from the MCV list, and the range estimators count it again inside
whichever bucket holds it.

The bucket count had to move with the population, not just the population. A
150-distinct column with 100 most-common values has 50 left; asking
percentile_disc for a fixed target+1 fractions would return 101 bounds,
mostly duplicates, describing buckets that hold no rows.

One merged check moved. skew's minimum is 0, which occurs five times and
is therefore now a most-common value, so the histogram no longer starts there.
That check asserted the column minimum; it now asserts the smallest
non-most-common value, derived from the written list rather than hardcoded.

A defect that was already there and invisible

Core sizes both lists from the column's attstattarget (:1065, :1897); this
function read the global default_statistics_target, so
ALTER TABLE ... SET STATISTICS was ignored. This suite has been setting it on
skew since slice 3 while asserting nothing about it, so the divergence was
already present in the file that should have caught it.

A target of zero means "do not collect", and is now honoured. Requesting only
zero-target columns also no longer raises collected statistics for no columns
with a hint blaming missing row groups, which pointed at storage for what was a
deliberate setting.

The harness, and why it is not a value comparison

pg_restore_attribute_stats takes VARIADIC "any" and answers a mistyped
argument with a WARNING rather than an error (attribute_stats.c:247-251):
most_common_freqs is real[], and a float8[] there is dropped while the call
reports success. Values cannot be the oracle, since exact and sampled statistics
differ by design -- that is the feature. So test/analyze_differential.sh
compares shape against core across five column types: which statistic kinds
are present, their operator and collation, plus every stored value verified
against an independent count.

Two text values in the fixture (alpha,beta and it's here) exercise the array
literal, which is why array_agg(...)::text is used rather than assembling one.

Proof by removal, which is the only reason to believe any of it

defect injected	result
most_common_freqs as float8[]	10 WARNINGs, all five MCV lists dropped, no ERROR -- the silent write, caught
frequency over non-null rows	3 checks in the value suite, 5 in the harness
histogram exclusion removed	4 checks, incl. 39 MCVs found inside the histogram
attstattarget ignored	RED before implementing: 101 bounds vs 11, 100 MCVs vs <=10

Two things I got wrong, since they shaped the result

The first draft of the slice 3b checks scored three of four passes against a
function that wrote no most-common values at all. They read core's leftover
list from the ANALYZE above them; most_common_vals matched because core had
already written {7,42,99}, and 99's frequency matched because core's sample
happened to round to 0.054. Statistics are now cleared before our call, so
attribution is structural rather than lucky.

The harness had the blind spot it exists to prevent. Its fixture had no
nulls, so both frequency denominators agreed and it passed with that defect
injected while the value suite caught it. Found by running the removal proof
rather than by reading. It is now one-in-seven null and catches it on all five
types.

I also wrote an element-type check that compared pg_typeof(stavalues1) against
the column type and reported all nine slots as mismatched. stavalues1 is
declared anyarray (pg_statistic.h:119), so pg_typeof returns the static
type for every row ever stored -- it fails identically against core's own
statistics. Replaced with the per-value verification, which observes something
real.

Verification

analyze_function.sh 38 checks, analyze_differential.sh 26, both green on
PG18 and PG19. Full 132-suite matrix on both majors. 15 through 17 skip cleanly
(exit 66, no checks) since pg_restore_attribute_stats arrived in 18.

One false red worth recording: the first PG19 run failed all five suites with
pg_ctl exhausting its 8 start retries, including suites this branch does not
touch. It was the per-major object-reuse footgun -- a PG18-built .so relinked
for PG19 -- not the code. A fresh per-major tree passes, which is what
run_all_versions.sh does.

Not in this PR

• Slices 4 and 5, as scoped.
• pgcolumnar.analyze() has no entry in docs/sql-reference.md at all, from
  slices 1-3. Worth its own change once the slices are done.
• The catch that should still decide the feature: autoanalyze will never call
  this, so these statistics rot silently into bad plans. Unresolved, Future: a background worker to schedule columnar vacuum and analyze, which autovacuum cannot reach #415.

🤖 Generated with Claude Code

https://claude.ai/code/session_013sY1RQR5MjZNUixA2um31r

[jdatcmd]

The PG18 red was real, and it was not this branch

harness_selftest failed with unregistered: parquet_nested_import on a run whose own summary lists that suite as PASS. Both are in the same log, so the check was wrong rather than the registration.

harness_selftest.sh: line 209: printf: write error: Broken pipe
FAIL  every suite is registered in run_all_versions.sh: got [unregistered: parquet_nested_import]

The check is:

listed_suites | grep -qx "$name" || unregistered="$unregistered $name"

The file runs under set -o pipefail. grep -q returns the instant it matches, closing the pipe while printf is still writing; printf takes EPIPE and exits non-zero, and pipefail reports the pipeline as failed even though grep matched. A registered suite is recorded as unregistered.

This contradicts the diagnosis recorded in that file. The note above the cache attributes this exact symptom in the #473 matrix -- innocent suites, different names each run, only some majors -- to transient fork failures. Caching removed the forks and the symptom survived. A race between grep exiting and printf finishing explains every property including why it never reproduces locally.

Measured rather than argued, 4,000 names matching the first, 200 attempts:

	false negatives
with pipefail	18 / 200
without	0 / 200

At the real list size it does not reproduce locally -- 132 short names fit the pipe buffer, so printf's write completes before grep can exit. That is consistent with it having been seen once, under CI load, on a name near the end of the list. I am not claiming a local reproduction at production size; the mechanism rests on the 4,000-name measurement plus the two contradictory halves of the CI log.

Fixed by removing the pipe: membership is now a case over the cached string, with newlines both sides for the whole-line match grep -x gave. No reader, no EPIPE, no race at any size.

Three controls added, since a matcher that always matched would also have made the red disappear while making the check useless: a registered name must be found, an absent one must not be, and a prefix of a registered name must not count. Proven by removal -- deleting analyze_differential from SUITES fails the check with that name.

43 checks green on PG18 and PG19. This is #469's territory rather than #414's, but it was blocking this PR and it is a false red that would keep naming innocent suites.

[jdatcmd]

CI is green on the current head, checked against the run rather than the badge: PR head 1e654ee, run 31187235383 head_sha 1e654ee, conclusion success. All 11 checks pass, including suites (PG 18) which carried the false red. Local 132-suite matrix green on PG18 and PG19.

Not self-merging. This PR changes behaviour you merged in slice 3 -- the histogram exclusion moves skew's bottom bound off the column minimum, and that check now derives its expectation from the written MCV list. It also corrects a diagnosis recorded in harness_selftest. Both are yours to disagree with.

The two things I would look at hardest if I were reviewing this:

1. The frequency denominator. I claim core divides by total rows including nulls (analyze.c:2720, track[i].count / samplerows) and built the fixture so the wrong denominator gives 0.2 where the truth is 0.18. If you read samplerows differently, the fixture is what has to change, not just the code.

2. attstattarget was out of scope and I did it anyway. It is a real divergence from core and it affects the list length this slice adds, so leaving it would have shipped MCVs sized by the wrong target. Say if you would rather it were split out.

One gap I did not fill: pgcolumnar.analyze() still has no entry in docs/sql-reference.md at all, from slices 1-3. Worth its own change once slices 4-5 land.

[ChronicallyJD]

Post-merge review of #476: both points you flagged hold, and here is the independent check

You asked to hear disagreement late rather than not at all. I do not have any on either point —
but "no objection" is worth as little as a green suite unless I say what I checked, so:

1. The frequency denominator includes NULLs. Confirmed, and there is a structural argument

You read analyze.c:2720 as dividing by total rows including nulls. That is right, and it does not
rest on reading samplerows charitably. The same variable is the denominator of null_frac in the
same function:

mcv_freqs[i] = (double) track[i].count / (double) samplerows;   /* MCV frequency  */
stats->stanullfrac = (double) null_cnt / (double) samplerows;   /* null_frac      */

null_cnt and nonnull_cnt are both incremented from the samplerows loop, so samplerows is
provably the whole sampled population. Two statistics sharing one denominator is what makes

null_frac + Σ mcv_freqs + (everything else) = 1

hold, and that identity is the thing the planner depends on: eqsel reads an MCV frequency as a
selectivity over all rows of the relation, then subtracts null_frac and the MCV total to price
what is left. Divide by the non-null count instead and every frequency is scaled by 1/(1-null_frac)
— still ordered, still summing to under one, still plausible, and the residual the planner computes
for non-MCV values goes negative-ish on a column with many nulls. Silent, as you said.

Same expression appears in compute_distinct_stats and compute_scalar_stats, so there is no
type-dependent variation to worry about.

Your fixture pinning it at 0.2-vs-0.18 is the right shape: the two denominators cannot agree, so the
check cannot pass by luck. The one-in-seven-null fix to the differential harness matters more than
the value suite here — a fixture with no nulls makes the two denominators identical, which is
exactly the blind spot you found by running the removal proof rather than reading it.

2. analyze_mcv_list — confirmed verbatim

if (samplerows == totalrows || totalrows <= 1.0)
    return num_mcv;

It is the first statement in the function, before ndistinct_table is even re-extracted. So reading
the column does remove the significance filter rather than requiring you to reimplement the
continuity-corrected Wald interval. Agreed that this is core's own answer and not a shortcut.

3. attstattarget being out of scope: do not split it back out

You flagged this as scope creep and offered to separate it. I would leave it, and not only on
convenience grounds — your version is better than the one I was carrying, in a way that is worth
recording:

	mine	merged
attstattarget NULL (PG18 default)	< 0 → NULL → guard skipped silently	coalesce(attstattarget, deftarget)
attstattarget = 0	wrote statistics anyway	honoured — column skipped

The second row is the one I would not have got to. A zero is a DBA instruction, and writing
statistics over it hands the planner numbers somebody deliberately turned off. Shipping MCVs sized by
the wrong target while leaving that in place would have been the worse trade.

Splitting it out now would mean a PR whose only content is re-deriving a fix that is already correct
and already covered.

What I did not review

#475 and #476 both merged without a recorded review, so this is post-merge and I have not run the
suites myself on this branch — I read the merged source and checked the three claims above against
upstream analyze.c. The 38 + 26 check counts are yours, not mine to vouch for.

I have one substantive thing outstanding on the merged function, which is a performance shape rather
than a defect, and it belongs in its own PR rather than in this thread. Writing it up separately.