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..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. @@ -129,16 +131,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 +565,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