From d096e7c3aad8abd594b1f69d06fe73b9e13f2e4e Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Fri, 7 Aug 2026 12:27:48 -0600 Subject: [PATCH 1/2] fix(#414, #485): place histogram bounds at core's positions, and count null_frac over live rows 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. 1. histogram_bounds were quantiles, not core's positions --------------------------------------------------------- core's 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, which is a different index whenever frac(i*nv/nfrac) is small, and a different VALUE whenever that shift crosses a value boundary. So ask 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. The exact boundary (T+1)/nv is a double, and nv in the millions leaves ~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 rows the histogram is actually placed over -- is now returned by 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. Taking nv from totalrows minus a zone-map null_frac would have put a rounded float in a position index. 2. null_frac counted rows a DELETE had already removed (#485) ------------------------------------------------------------- null_frac 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 leaves a true null_frac of 0.1431 and a zone-map null_frac of 0.1000 -- 30% low, and VACUUM does not heal it. 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: null_frac 0.100000 implies a 1,200-row table most_common_freqs[1] 0.2667 implies a 900-row table 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. 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 before either judges the code. Verified to discriminate, rather than assumed to: run against stock origin/main the new checks fail 3 of 8 -- exactly the three behavioural ones, with all five premise checks still passing, so the premises are not doing the work. new tests + this change 46 checks, PASSED new tests + origin/main 46 checks, FAILED (3) analyze_function 46, analyze_differential 26, analyze_stats 27, all green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QRQYekvivA4RLDnndhanHK --- pgcolumnar--1.0-alpha.sql | 133 +++++++++++++++++++++++++++++-------- test/analyze_function.sh | 136 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 234 insertions(+), 35 deletions(-) diff --git a/pgcolumnar--1.0-alpha.sql b/pgcolumnar--1.0-alpha.sql index f9771da..c7d2da4 100644 --- a/pgcolumnar--1.0-alpha.sql +++ b/pgcolumnar--1.0-alpha.sql @@ -1012,6 +1012,10 @@ DECLARE orderable boolean; nmcv integer; nremaining bigint; + nullcount bigint; /* live rows with no value, from the same read */ + nonnull bigint; /* rows with a value, from the aggregation below */ + mcvrows bigint; /* of those, the rows the MCV list holds */ + nv bigint; /* the population the histogram is placed over */ nfrac integer; -- The per-column target, resolved inside the loop. attstattarget is NULL when -- the column has never been given one, and core reads that as "use the global @@ -1105,37 +1109,59 @@ BEGIN END IF; nbuckets := coalesce(att.attstattarget, deftarget); /* - * null_frac, exactly. value_count counts the values present and - * null_count those absent, so the denominator is their sum rather than - * value_count alone. vector_index = -1 is the whole-chunk aggregate; - * summing the per-vector rows as well would double count. + * Has this column been written yet? The zone maps answer that and + * nothing else here. + * + * They used to answer null_frac as well -- + * sum(null_count) / sum(value_count + null_count) -- and that was wrong + * after a DELETE. Those counts describe what was WRITTEN; deleting a row + * marks it dead without rewriting them, so the denominator keeps counting + * rows the table no longer holds. On 1,000 rows with 100 nulls, deleting + * the 301 rows holding one value leaves a true null_frac of 0.1431 and a + * zone-map null_frac of 0.1000, a 30% understatement that VACUUM does not + * heal. Worse than the size of the error: null_frac came from the zone + * maps while the most-common-value frequencies came from count(*), so the + * two were normalised against different populations and + * null_frac + sum(mcv_freqs) + rest = 1 -- the identity the planner's + * selectivity arithmetic rests on -- silently stopped holding. + * + * So the fraction is taken from the same read as everything else below, + * and the zone maps keep only the job they can still do exactly: telling + * us whether there are any row groups at all. * * column_index is the 0-based attribute position. attnum is stable * across a dropped column, so attnum - 1 keeps pointing at the same * column after a DROP COLUMN. */ - SELECT sum(z.null_count)::double precision - / nullif(sum(z.value_count + z.null_count), 0) - INTO nullfrac + PERFORM 1 FROM pgcolumnar.zone_map z WHERE z.storage_id = sid AND z.column_index = att.attnum - 1 AND z.vector_index = -1; - CONTINUE WHEN nullfrac IS NULL; /* no zone map rows: nothing exact to say */ + CONTINUE WHEN NOT FOUND; /* no zone map rows: nothing exact to say */ /* - * n_distinct, exactly, by reading this column and nothing else. This is - * the whole point of the function: on the 3M x 20 fixture a projected - * single-column read costs 268 ms where core's whole-table sample costs - * 6,302 ms, because core's fixed 30,000-row sample lands in every row - * group and so decodes every column of the table. + * n_distinct, the row count and the null count, by reading this column + * and nothing else. This is the whole point of the function: on the + * 3M x 20 fixture a projected single-column read costs 268 ms where + * core's whole-table sample costs 6,302 ms, because core's fixed + * 30,000-row sample lands in every row group and so decodes every column + * of the table. * - * count(DISTINCT) ignores NULLs, which is what n_distinct means. + * count(DISTINCT) ignores NULLs, which is what n_distinct means. The + * null count comes from the same scan so that it cannot disagree with the + * denominator the frequencies below are divided by. */ - EXECUTE format('SELECT count(DISTINCT %I)::bigint, count(*)::bigint FROM %I.%I', - att.attname, schname, relnm) - INTO ndistinct, totalrows; + EXECUTE format('SELECT count(DISTINCT %I)::bigint, count(*)::bigint,' + ' count(*) FILTER (WHERE %I IS NULL)::bigint' + ' FROM %I.%I', + att.attname, att.attname, schname, relnm) + INTO ndistinct, totalrows, nullcount; + + nullfrac := CASE WHEN totalrows > 0 + THEN nullcount::double precision / totalrows::double precision + ELSE 0 END; /* * Core's own convention, and the sign is load-bearing: positive is an @@ -1207,18 +1233,33 @@ BEGIN */ mcvvals := NULL; mcvfreqs := NULL; + nonnull := 0; + mcvrows := 0; IF orderable THEN + /* + * The same aggregation, split into the full group and the most-common + * slice of it, so it can also report how many ROWS each covers. The + * histogram below is built over the non-null rows the MCV list does + * NOT hold, and it has to know how many those are to place a bound at + * a position rather than at a fraction. + * + * Both counts come from this one aggregation rather than from the zone + * maps or a second scan, so the population the histogram is placed + * over is by construction the population the MCV list was taken from. + */ EXECUTE format( - 'SELECT array_agg(v ORDER BY c DESC, v)::text, - array_agg((c::double precision / %s::double precision)::real - ORDER BY c DESC, v) - FROM (SELECT %I AS v, count(*)::bigint AS c - FROM %I.%I WHERE %I IS NOT NULL - GROUP BY 1 HAVING count(*) > 1 - ORDER BY count(*) DESC, 1 - LIMIT %s) t', - totalrows, att.attname, schname, relnm, att.attname, nbuckets) - INTO mcvvals, mcvfreqs; + 'WITH g AS MATERIALIZED (' + ' SELECT %I AS v, count(*)::bigint AS c' + ' FROM %I.%I WHERE %I IS NOT NULL GROUP BY 1),' + ' m AS MATERIALIZED (' + ' SELECT v, c FROM g WHERE c > 1 ORDER BY c DESC, v LIMIT %s)' + 'SELECT (SELECT array_agg(v ORDER BY c DESC, v)::text FROM m),' + ' (SELECT array_agg((c::double precision / %s::double precision)::real' + ' ORDER BY c DESC, v) FROM m),' + ' (SELECT coalesce(sum(c), 0)::bigint FROM g),' + ' (SELECT coalesce(sum(c), 0)::bigint FROM m)', + att.attname, schname, relnm, att.attname, nbuckets, totalrows) + INTO mcvvals, mcvfreqs, nonnull, mcvrows; END IF; /* @@ -1263,10 +1304,13 @@ BEGIN nmcv := coalesce(array_length(mcvfreqs, 1), 0); nremaining := ndistinct - nmcv; + nv := nonnull - mcvrows; + hist := NULL; IF att.attnum > 0 AND orderable AND nremaining >= 2 + AND nv > 1 THEN /* * least(nbuckets, nremaining - 1) fractions, so the bound count is @@ -1274,6 +1318,36 @@ BEGIN */ nfrac := least(nbuckets, nremaining - 1); + /* + * A bound is a POSITION, not a quantile, and the difference is not + * academic. core's 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 fraction p to index ceil(p * nv) - 1, which + * is a different index whenever frac(i*nv/nfrac) is small, and a + * different VALUE whenever that shift crosses a value boundary. On a + * column with many rows per distinct value the two agree and the + * distinction is invisible; on eleven distinct rows at a statistics + * target of 3 they disagree at the third bound, 8 against 7. + * + * So ask percentile_disc for the fractions that resolve to core's + * positions instead of 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 up to a few million leaves roughly + * 1e-9 of slack in p*nv; landing a hair above T+1 makes ceil() return + * T+2 and takes the NEXT value. Half a row of margin cannot be crossed + * by that error, and any p in (T/nv, (T+1)/nv] resolves to T. + * + * nv is the count from the aggregation above, not a derived figure: + * deriving it as totalrows minus a null_frac read off the zone maps + * would put a rounded float in a position index. + */ + /* * The exclusion is a literal list rather than a re-aggregation. The * alternative -- recomputing the most-common set in a subquery -- is @@ -1284,11 +1358,12 @@ BEGIN */ EXECUTE format( 'SELECT percentile_disc( - (SELECT array_agg(i::double precision / %s ORDER BY i) + (SELECT array_agg(((floor(i::numeric * (%s - 1) / %s) + 0.5) + / %s)::double precision ORDER BY i) FROM generate_series(0, %s) i)) WITHIN GROUP (ORDER BY %I)::text FROM %I.%I WHERE %I IS NOT NULL %s', - nfrac, nfrac, att.attname, schname, relnm, att.attname, + nv, nfrac, nv, nfrac, att.attname, schname, relnm, att.attname, CASE WHEN mcvvals IS NULL THEN '' ELSE format('AND %I <> ALL (%L::%s[])', att.attname, mcvvals, format_type(att.atttypid, NULL)) diff --git a/test/analyze_function.sh b/test/analyze_function.sh index 05fcdf6..b4e4961 100755 --- a/test/analyze_function.sh +++ b/test/analyze_function.sh @@ -129,16 +129,20 @@ fi # --- check 1: pgcolumnar.analyze() gives the EXACT null_frac ------------------- # -# The zone maps already hold null_count and value_count per chunk, so this is a -# metadata read: 11 ms on the 1237 MB fixture, against 6,302 ms for core ANALYZE. -# Exactness is a by-product of that, not the reason for it -- the reason is the -# 23.5x. But it is the cheapest thing to assert that a sampled implementation -# cannot fake. +# Exact because the column is read rather than sampled, which is the thing a +# sampled implementation cannot fake. +# +# This used to come from the zone maps, which was cheaper -- a metadata read +# rather than a scan -- and wrong after a DELETE (#485): those counts describe +# what was WRITTEN, and deleting a row does not rewrite them. It now comes from +# the same read as n_distinct, which costs nothing extra because that read +# happens either way, and which cannot disagree with the denominator the +# most-common frequencies are divided by. The delete case is checked below. psql_run "SELECT pgcolumnar.analyze('af_c'::regclass, ARRAY['k']);" >/dev/null ours_nullfrac="$(q "SELECT null_frac FROM pg_stats WHERE tablename = 'af_c' AND attname = 'k'")" -check_num "pgcolumnar.analyze() reports null_frac exactly, from the zone maps" \ +check_num "pgcolumnar.analyze() reports null_frac exactly, from reading the column" \ "$ours_nullfrac" "0.1" # --- check 2: n_distinct is exact, from reading one column -------------------- @@ -559,4 +563,124 @@ check "a column at SET STATISTICS 0 is left alone, because that is what zero mea WHERE tablename = 'af_c' AND attname = 'pad1'), '')")" \ "" +# --- histogram bounds are POSITIONS, not quantiles (#414 follow-on) ----------- +# +# core's 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, which is a different index and +# therefore a different VALUE whenever the shift crosses a value boundary. +# +# The 500,000-row fixtures above cannot show the difference: with many rows per +# distinct value a one-row shift lands on the same value, so both algorithms +# agree and the check would pass either way. A fixture that cannot distinguish +# the two implementations cannot test them. Eleven distinct rows at a statistics +# target of 3 can: +# +# nv = 11, nhist = 4, so the divisor is 3 +# stride i=2 -> floor(2*10/3) = 6 -> the 7th value = 7 +# percentile_disc i=2 -> ceil(2*11/3)-1 = 7 -> the 8th value = 8 +# +# The expectation is NOT taken from the implementation. It is computed by the +# oracle below, straight from core's formula over row_number(), and it is also +# hand-workable: the values are 1..11 each appearing once, so position p holds +# value p+1 and the bounds are {1,4,7,11}. Two independent derivations that agree +# with each other before either judges the code. +psql_run "DROP TABLE IF EXISTS af_h11; + CREATE TABLE af_h11 (v int) USING pgcolumnar; + INSERT INTO af_h11 SELECT generate_series(1, 11); + ALTER TABLE af_h11 ALTER COLUMN v SET STATISTICS 3;" >/dev/null + +check_num "premise: the stride fixture has no repeated value, so no MCV is excluded" \ + "$(q "SELECT count(*) FROM (SELECT v FROM af_h11 GROUP BY v HAVING count(*) > 1) t")" "0" + +h11_oracle="$(q "WITH nonmcv AS ( + SELECT v, row_number() OVER (ORDER BY v) - 1 AS pos + FROM af_h11 WHERE v IS NOT NULL), + n AS (SELECT count(*)::bigint AS nv FROM nonmcv), + p AS (SELECT floor(i::numeric * (n.nv - 1) / (4 - 1))::bigint AS pos + FROM generate_series(0, 3) i, n) + SELECT (SELECT array_agg(v ORDER BY pos) FROM nonmcv + WHERE pos IN (SELECT pos FROM p))::text")" + +# The oracle and the hand-worked figure are derived separately. If they ever +# disagree the oracle is wrong, and nothing below it means anything. +check "premise: the independent oracle agrees with the hand-worked bounds" \ + "$h11_oracle" "{1,4,7,11}" + +psql_run "SELECT pgcolumnar.analyze('af_h11'::regclass, ARRAY['v']);" >/dev/null + +check "premise: a histogram was written, so the comparison below is not vacuous" \ + "$(q "SELECT CASE WHEN histogram_bounds IS NULL THEN 'none' ELSE 'present' END + FROM pg_stats WHERE tablename = 'af_h11' AND attname = 'v'")" "present" + +check "histogram_bounds are core's positional stride, not evenly spaced quantiles" \ + "$(q "SELECT histogram_bounds::text FROM pg_stats + WHERE tablename = 'af_h11' AND attname = 'v'")" "$h11_oracle" + +# --- null_frac describes the rows the table HOLDS (#485) ---------------------- +# +# It used to come from the zone maps, which count what was written. A DELETE +# marks rows dead without rewriting those counts, so the fraction stayed +# normalised against a population the table no longer had -- and VACUUM did not +# clear it. +# +# The consequence is worse than the size of the error. 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 +# and null_frac + sum(mcv_freqs) + rest = 1 stopped holding. eqsel subtracts both +# to price everything else, so the residual it computes went wrong by the +# difference. +# +# 1,200 rows: 120 null, 300 holding 7, 300 holding 9, the rest unique. Deleting +# the rows holding 9 leaves 900 rows and keeps 7 in the most-common list, so both +# denominators are observable in the same written row. +psql_run "DROP TABLE IF EXISTS af_del; + CREATE TABLE af_del (v int) USING pgcolumnar; + INSERT INTO af_del + SELECT CASE WHEN i % 10 = 0 THEN NULL + WHEN i % 4 = 0 THEN 7 + WHEN i % 4 = 1 THEN 9 + ELSE 100000 + i END + FROM generate_series(1, 1200) i; + DELETE FROM af_del WHERE v = 9;" >/dev/null + +check_num "premise: the delete left fewer live rows than the zone maps describe" \ + "$(q "SELECT CASE WHEN (SELECT count(*) FROM af_del) + < (SELECT sum(z.value_count + z.null_count) + FROM pgcolumnar.zone_map z + JOIN pgcolumnar.storage s ON s.storage_id = z.storage_id + WHERE s.relation_oid = 'af_del'::regclass + AND z.column_index = 0 AND z.vector_index = -1) + THEN 1 ELSE 0 END")" "1" + +psql_run "SELECT pgcolumnar.analyze('af_del'::regclass, ARRAY['v']);" >/dev/null + +# 120 nulls in 900 live rows. Against the zone maps this read 0.1. +check "null_frac counts live rows, not rows a DELETE left behind" \ + "$(q "SELECT round(null_frac::numeric, 6)::text FROM pg_stats + WHERE tablename = 'af_del' AND attname = 'v'")" "0.133333" + +check_num "premise: 7 survived the delete and is still a most-common value" \ + "$(q "SELECT CASE WHEN most_common_vals::text::int[] @> ARRAY[7] THEN 1 ELSE 0 END + FROM pg_stats WHERE tablename = 'af_del' AND attname = 'v'")" "1" + +# The point of the pair: both statistics must imply the same table. Divide each +# by the count it describes and the row count that falls out must be the real +# one, from both directions. +check "null_frac and the most-common frequencies agree on how many rows there are" \ + "$(q "SELECT CASE WHEN + round((SELECT count(*) FILTER (WHERE v IS NULL) FROM af_del)::numeric + / nullif(null_frac::numeric, 0)) = (SELECT count(*) FROM af_del) + AND round((SELECT count(*) FILTER (WHERE v = 7) FROM af_del)::numeric + / nullif((most_common_freqs)[1]::numeric, 0)) = (SELECT count(*) FROM af_del) + THEN 'yes' ELSE 'no (' + || round((SELECT count(*) FILTER (WHERE v IS NULL) FROM af_del)::numeric + / nullif(null_frac::numeric, 0))::text || ' vs ' + || round((SELECT count(*) FILTER (WHERE v = 7) FROM af_del)::numeric + / nullif((most_common_freqs)[1]::numeric, 0))::text || ')' END + FROM pg_stats WHERE tablename = 'af_del' AND attname = 'v'")" "yes" + pgc_summary From c19f9b00a6de9a15d88f3ac39da7e193fe839ae1 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Fri, 7 Aug 2026 12:52:21 -0600 Subject: [PATCH 2/2] docs(#485): the suite header still claimed null_frac comes from the zone maps Found by trial-merging #490, which touches the same file: the two merge cleanly and neither owns this line, so the merged suite would have carried a header contradicting its own check. The claim is invalidated by this branch, so it is this branch's to fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QRQYekvivA4RLDnndhanHK --- test/analyze_function.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/analyze_function.sh b/test/analyze_function.sh index b4e4961..0d769fe 100755 --- a/test/analyze_function.sh +++ b/test/analyze_function.sh @@ -28,9 +28,11 @@ # This suite is about the function, not the AM sampler. test/analyze_stats.sh # covers the sampler (#154) and stays the correctness path for plain ANALYZE. # -# Slice 1 asserts null_frac comes from the zone maps and is EXACT where core's -# is sampled. Slice 2 asserts n_distinct is exact from reading ONE column, which -# is the claim the whole issue rests on. +# Slice 1 asserts null_frac is EXACT where core's is sampled. It came from the +# zone maps until #485: those counts describe what was WRITTEN, so a DELETE left +# the fraction normalised against rows the table no longer held. It now comes +# from the same read as n_distinct. Slice 2 asserts n_distinct is exact from +# reading ONE column, which is the claim the whole issue rests on. # # Usage: test/analyze_function.sh [PG_CONFIG] # Written fresh for pgColumnar.