Place histogram bounds at core's positions, and count null_frac over live rows (#414, #485)

[ChronicallyJD]

Closes #485. Follows on from #475 and #476.

Two defects in pgcolumnar.analyze(), both found at the same seam — call the function, read pg_stats — and both of the shape this function keeps producing: a plausible value, written successfully, with nothing raising.

These are separable and I will split them if you would rather. They land together because they touch adjacent lines of the same loop and the same suite, and because the second one needs a row count the first one already computes. #485 is the one with a user-visible wrong answer today, so if you want it alone and fast, say so and I will send it on its own.

1. histogram_bounds were quantiles, not core's positions

compute_scalar_stats places bound i at

values[floor(i * (nvals - 1) / (num_hist - 1))]

among the rows left after the most-common values are removed. percentile_disc resolves a fraction p to index ceil(p * nv) - 1 — a different index whenever frac(i*nv/nfrac) is small, and a different value whenever that shift crosses a value boundary.

So the fix asks percentile_disc for the fractions that land on core's positions rather than for evenly spaced quantiles:

p_i = (floor(i * (nv - 1) / nfrac) + 0.5) / nv

The half is load-bearing rather than decorative. The exact boundary (T+1)/nv is a double, and nv in the millions leaves roughly 1e-9 of slack in p*nv; landing a hair above T+1 makes ceil() return T+2 and take the next value. Half a row of margin cannot be crossed by that error.

nv — the population the histogram is actually placed over — now comes back from the most-common aggregation, which is split into the full group and its most-common slice so it can report how many rows each covers. Deriving nv as totalrows minus a zone-map null_frac would have put a rounded float in a position index.

This changes the emitted array. Same length, same endpoints (position 0 is still the true minimum and nv-1 the true maximum, so #414's exactness claim is untouched); interior bounds can shift by one position. Both forms are valid equi-depth histograms — the case for core's is that it is what the planner's selectivity estimators were tuned against, and that it is the form computable from grouped data, which is where the follow-on performance work goes.

2. null_frac counted rows a DELETE had already removed (#485)

It came from the zone maps, which count what was written. Deleting a row marks it dead without rewriting those counts, so the denominator kept counting rows the table no longer held. On 1,000 rows with 100 nulls, deleting the 301 rows holding one value:

	null_frac
truth (100/699)	0.143062
written	0.100000
after VACUUM	0.100000

The size of the error is not the worst of it. null_frac came from the zone maps while the most-common frequencies came from count(*), so one pg_stats row carried two statistics normalised against different populations:

	value	implies a table of
null_frac	0.100000	1,200 rows
most_common_freqs[1]	0.2666667	900 rows
rows actually present		900

null_frac + sum(mcv_freqs) + rest = 1 stopped holding, and eqsel subtracts both to price everything else.

The null count now comes from the read the function was already doing — count(*) was in that query already, so this is one more aggregate over a scan that happens either way. The zone maps keep the one job they can still do exactly: telling us whether the column has any row groups at all.

This gives up "null_frac is a metadata read". That was #414 slice 1's stated selling point and it is worth saying out loud rather than deleting quietly. It cost nothing here because the function never collects null_frac alone — it always goes on to read the column for n_distinct. If you want a metadata-only fast path later, the honest version needs a live-row count, which is the same scan.

The wider question — whether pgcolumnar.zone_map's counts should account for the delete vector, which would touch pruning too — is real and is not addressed here.

Tests

Eight checks in test/analyze_function.sh, at the public seam.

The existing 500,000-row fixtures cannot show defect 1: with many rows per distinct value a one-row shift lands on the same value, so both algorithms agree and a check would pass either way. Eleven distinct rows at a statistics target of 3 separate them — stride takes position 6, percentile_disc position 7.

The expected bounds are not taken from the implementation. They are computed by an independent SQL oracle from core's formula over row_number(), and separately worked by hand ({1,4,7,11}); a premise check asserts those two agree with each other before either judges the code.

The checks were verified to discriminate, not assumed to

Running the new tests against stock origin/main:

new tests + this change     46 checks, PASSED
new tests + origin/main     46 checks, FAILED

FAIL  histogram_bounds are core's positional stride, not evenly spaced quantiles: got [{1,4,8,11}] want [{1,4,7,11}]
FAIL  null_frac counts live rows, not rows a DELETE left behind: got [0.100000] want [0.133333]
FAIL  null_frac and the most-common frequencies agree on how many rows there are: got [no (1200 vs 900)] want [yes]

Exactly the three behavioural checks fail, and all five premise checks still pass in both arms — so the premises are describing the fixture rather than doing the discriminating.

Gate

suite	PG18.4	PG19beta2
analyze_function	46 ✅	46 ✅
analyze_differential	26 ✅	26 ✅
analyze_stats	27 ✅	27 ✅

make clean between the two majors — without it make relinks the previous major's objects and the PG19 .so still references get_relation_info_hook, which PG19 removed in favour of build_simple_rel_hook. The version guards in columnar_tableam.c are correct and test/run_all_versions.sh cleans per major (line 556), so the project's gate does not have this hole; an ad-hoc single-suite run across majors in one tree does. Confirmed the PG19 .so by symbol before trusting the run.

Interaction with #487

#487 is about the flaky premise above check 1. This PR renames that check — it now reads "reports null_frac exactly, from reading the column" rather than "from the zone maps", because the mechanism it named is gone. It does not touch the flaky premise itself, so #487 stands as filed and the two will conflict on that region if both are in flight. Happy to rebase on whichever lands first.

Not in this PR

The performance work that started this. Main aggregates the same column twice — count(DISTINCT) and then a GROUP BY for the most-common list — and the grouped pass already holds both answers. That is behaviour-preserving, so it has no failing test to drive it, and it belongs in its own PR with the measurement rather than smuggled in beside two correctness fixes.

[jdatcmd]

Not a review yet, just clearing a hazard so the review is about the code.

Your green predates two merges into the file you touch. This PR's CI ran at 18:52:28 on c19f9b0, which is still the head, so the run is fresh for your branch. But main moved underneath it three minutes later:

	merged
#490 (#487, rewrote analyze_function.sh's premises, added a k7 column to af_c)	18:55:39
#494 (#483, the reader's cross-type resolution)	19:46:11

mergeStateStatus: CLEAN only says the text merges. Both of us edited the same suite and the same fixture, so that is not the interesting question.

I merged it locally and ran it, so it is answered rather than assumed:

• The merge is clean, no conflicts, and the result keeps both sets of work: af_c carries my k7 column alongside your stride and live-null_frac changes.
• PG18: 50 checks, PASSED. PG19: 50 checks, PASSED.
• My arithmetic premise from analyze_function's null_frac premise fails ~1 run in 130: it requires core's sample to miss its own mode #487 still holds against your changed null_frac source, which was the one I would have expected to break: core sampled 0.13876666 for k7 against a truth of 0.142856, so the "no whole number of sampled rows gives that fraction" premise is still doing its job rather than passing by luck.

So there is nothing to rebase for and nothing of mine that your change breaks.

On splitting: my instinct is not to. #485 is the one with a user-visible wrong answer, so there is a real argument for landing it alone and fast, but the two are green together, they are reviewed together, and splitting now costs a full CI round plus a second review for no evidence gained. If you have a reason to want #485 in isolation that I cannot see from here, say so and I will take it that way.

I will come back with an actual review of the substance. The two parts I want to spend time on are the changed histogram_bounds positions, since that is an emitted-value change the planner sees, and the + 0.5 margin argument, which I want to satisfy myself about independently rather than agree with because it reads convincingly.

[jdatcmd]

Both claims check out. I verified each against a source independent of your description rather than agreeing with the reasoning, and one of them I can now put a number on that you did not have.

1. Core's placement formula: confirmed against analyze.c

compute_scalar_stats does not write the closed form, it accumulates:

delta = (nvals - 1) / (num_hist - 1);
deltafrac = (nvals - 1) % (num_hist - 1);
pos = posfrac = 0;
for (i = 0; i < num_hist; i++) {
    hist_values[i] = values[pos].value;
    pos += delta; posfrac += deltafrac;
    if (posfrac >= (num_hist - 1)) { pos++; posfrac -= (num_hist - 1); }
}

That is Bresenham for values[(i * (nvals - 1)) / (num_hist - 1)] with C integer division, and core's own comment above the loop states exactly that closed form and says the accumulation exists only to avoid integer overflow at large stats targets. So floor(i * (nvals - 1) / (num_hist - 1)) is right, including that it is floor rather than round.

Two consequences worth recording, both of which your code already gets right:

• nvals there is rows remaining after MCV removal, not distinct values, which is why your nv has to be a row count and why deriving it from a zone-map null_frac would have been wrong for a second reason beyond the rounded float.
• Your numeric floor and core's C integer division agree. A ratio of integers can only sit within 1e-20 of an integer if it IS that integer, so numeric's rounding cannot cross a boundary that C truncation does not.

2. percentile_disc's index rule: confirmed, and the + 0.5 is load-bearing

Your PR uses the array form, so the code that runs is percentile_disc_multi_final -> setup_pct_info, which for the discrete case does:

int64 row = (int64) ceil(p * rowcount);
row = Max(1, row);
pct_info[i].first_row = row;      /* 1-based */

0-based index ceil(p*nv) - 1, as you said. So landing on T requires p*nv in (T, T+1]: (T+1)/nv is the upper endpoint, (T+0.5)/nv the midpoint.

I did not want to take "the margin is needed" on the argument, so I measured the failure rate. Computing both forms in float8 exactly as the server does, over every T for each nv:

nv	naive (T+1)/nv wrong	(T+0.5)/nv wrong
999	6	0
1000	0	0
4096	0	0
10007	948	0
100000	5336	0
999983	11571	0
1000000	11555	0
4000037	46205	0

About 1 bound in 87 at a million rows, zero for yours at every size tested. And end to end through the aggregate itself, on the first failing case the sweep found:

nv = 1000000, target index T = 122
  percentile_disc((122+1)/1000000)   -> 123     wrong
  percentile_disc((122+0.5)/1000000) -> 122     right

So the half is not defensive, it is the difference between correct and about one percent wrong, and the round-number cases (1000, 4096) are exactly the ones that would have made a hand-check of the naive form look fine. Worth having that table in the comment, or in the commit message, so nobody later reads + 0.5 as noise and simplifies it away.

Verdict

Approving. Not splitting, per my earlier comment, and I have nothing to add on the null_frac half beyond what I posted: the merge with current main is clean and green on PG18 and PG19, 50 checks each.

The one thing I would still like written down somewhere, and it does not block this: the histogram change alters an emitted value the planner consumes. Endpoints are unchanged so #414's exactness claim holds, but the CHANGELOG entry should say plainly that interior bounds can move by one position, so that anyone diffing pg_stats across an upgrade knows it is intended.