From 1d8b86b28d26336314a60f0bec38c33c700f8322 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 7 Aug 2026 08:05:47 -0600 Subject: [PATCH 1/2] feat: most-common values, exact and excluded from the histogram (#414 slice 3b) pgcolumnar.analyze() now collects most_common_vals and most_common_freqs from the same single-column read that already produced n_distinct and the histogram, and removes those values from the histogram. The selection rule is core's, and reading the whole column removes most of it. analyze_mcv_list() opens with if (samplerows == totalrows || totalrows <= 1.0) return num_mcv; analyze.c:2995 so the continuity-corrected Wald interval over a hypergeometric variance -- the entire significance filter -- is skipped when the table was read rather than sampled. That machinery decides whether a SAMPLE frequency can be trusted; we do not sample. What is left is mechanical: values with count > 1 are eligible (:2549), the top attstattarget of those by count (:2552), and a frequency over TOTAL rows including nulls (:2720). The last of those fails quietly, so the fixture is built to catch it: `cat` is one-tenth null, which makes count/total and count/non-null differ. Dividing by the non-null count yields 0.2/0.12/0.06 where the truth is 0.18/0.108/0.054 -- ordered, summing to less than one, plausible, wrong. Injecting that defect fails three checks. Excluding the most-common values from the histogram is required rather than cosmetic. Keeping them counts those values twice in selectivity: eqsel takes the frequency from the MCV list and the range estimators count it again inside the bucket holding it. Core collapses them out at :2768-2799 and sizes what remains at :2744, so the bucket count had to move with the population: asking for a fixed target+1 fractions after removing 100 values from a 150-distinct column would repeat bounds, describing buckets that hold no rows. This changes slice 3's behaviour rather than only adding to it. 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 and now asserts the smallest non-most-common value. Also fixes the per-column statistics target, which was already diverging and invisible. Core reads attstattarget (:1065, :1897) where this function read the global default, so ALTER TABLE ... SET STATISTICS was ignored -- this suite had been setting it on skew since slice 3 while asserting nothing about it. A target of zero means do not collect, and is now honoured; requesting only zero-target columns no longer raises an error whose hint blamed missing row groups. test/analyze_differential.sh is new, and lands with the slice rather than after it because the failure it catches is silent. pg_restore_attribute_stats takes VARIADIC "any" and answers a mistyped argument with a WARNING, not an error: 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, so it compares shape against core -- which kinds are present, their operator and collation -- across five column types, and verifies every stored value against an independent count. Injecting the float8[] defect produces ten WARNINGs, drops all five MCV lists, and raises nothing. Its own first draft had the blind spot it exists to prevent: with no nulls in the fixture the two frequency denominators agree, so it passed with the denominator defect injected while the value suite caught it. The fixture is now one-in-seven null and catches it on all five types. Gated PG18+19; 15 through 17 skip cleanly, since pg_restore_attribute_stats arrived in 18. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013sY1RQR5MjZNUixA2um31r --- CHANGELOG.md | 37 ++++ pgcolumnar--1.0-alpha.sql | 253 +++++++++++++++++++++------ test/analyze_differential.sh | 325 +++++++++++++++++++++++++++++++++++ test/analyze_function.sh | 259 +++++++++++++++++++++++++++- test/run_all_versions.sh | 1 + 5 files changed, 817 insertions(+), 58 deletions(-) create mode 100755 test/analyze_differential.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index e7d9589..db01948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,45 @@ which was true until that script existed. ## [Unreleased] +### Added + +- `pgcolumnar.analyze()` now collects `most_common_vals` and `most_common_freqs`, + and excludes those values from `histogram_bounds` (#414). Frequencies are exact + counts over the total row count rather than sample estimates. PostgreSQL 18 and + later, which is where `pg_restore_attribute_stats` exists; earlier majors raise + and should use `ANALYZE`. + + The selection rule is PostgreSQL's own. `analyze_mcv_list()` keeps the entire + list when the whole table was read instead of applying its significance filter, + because that filter exists to judge sample frequencies. Reading the column makes + the values eligible on count alone, matching what core would store given the + same information. + + Excluding most-common values from the histogram is required rather than + cosmetic: keeping them counts those values twice in selectivity, once from the + most-common list and again inside the bucket that holds them. + +- `test/analyze_differential.sh`, which compares the statistics `pgcolumnar.analyze()` + writes against the shape PostgreSQL's own `ANALYZE` produces across five column + types. `pg_restore_attribute_stats` takes `VARIADIC "any"` and responds to a + mistyped argument with a warning rather than an error, so a statistic can be + dropped while the call reports success. Values cannot be the comparison, since + exact and sampled statistics differ by design, so the suite compares the + operator, collation and presence of each statistic kind, and verifies every + stored value against an independent count. + ### Fixed +- `pgcolumnar.analyze()` now honours the per-column statistics target set by + `ALTER TABLE ... ALTER COLUMN ... SET STATISTICS` (#414). It read the global + `default_statistics_target` for every column, so a column given its own target + was sized by the global setting instead. A target of zero means the column is + not to be analysed at all, and is now respected rather than overridden. + + Requesting only zero-target columns no longer raises. The function reported + that it had collected statistics for no columns, with a hint about missing row + groups, which pointed at storage for what was a deliberate setting. + - Renamed the custom scan node from `ColumnarScan` to `PgColumnarScan`, and the custom path from `ColumnarAgg` to `PgColumnarAgg` (#428). `ColumnarScan` is also registered by **Citus columnar** and by **TimescaleDB 2.29**. diff --git a/pgcolumnar--1.0-alpha.sql b/pgcolumnar--1.0-alpha.sql index 00d16ed..f9771da 100644 --- a/pgcolumnar--1.0-alpha.sql +++ b/pgcolumnar--1.0-alpha.sql @@ -982,28 +982,47 @@ COMMENT ON FUNCTION pgcolumnar.parallel_copy(regclass, text, int) * an opt-in accelerator for wide tables and, like pgcolumnar.vacuum(), nothing * schedules it: see #415. * - * Collected so far: null_frac exactly from the zone maps (metadata only, no data - * read), and n_distinct exactly by reading one column. MCVs and histogram bounds - * still need a sample and are not collected here yet. + * Collected so far, all of it exact rather than sampled: null_frac from the zone + * maps (metadata only, no data read), n_distinct from reading one column, and + * from that same read the most-common values with their frequencies and a + * histogram of what remains once those are excluded. + * + * "Exact" is the whole difference and it is not a refinement of core's numbers. + * Core samples 30,000 rows, so a value held by one row in 500,000 is missed + * entirely and every range estimate above the sampled maximum collapses; a + * frequency is right to about three digits rather than exactly. Reading the + * column removes the sampling error rather than reducing it -- which is also why + * core's own significance filter for the most-common list does not apply here, + * as analyze_mcv_list() says itself at analyze.c:2995. */ CREATE FUNCTION pgcolumnar.analyze(rel regclass, columns text[] DEFAULT NULL) RETURNS void LANGUAGE plpgsql AS $$ DECLARE - sid bigint; - att record; - nullfrac double precision; - ndistinct bigint; - totalrows bigint; - ndstat double precision; - hist text; - -- One fewer than the number of bounds, matching core's default_statistics_target. - nbuckets integer := current_setting('default_statistics_target')::integer; - seen integer := 0; - unknown text; - schname text; - relnm text; + sid bigint; + att record; + nullfrac double precision; + ndistinct bigint; + totalrows bigint; + ndstat double precision; + hist text; + mcvvals text; + mcvfreqs real[]; + orderable boolean; + nmcv integer; + nremaining bigint; + 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 + -- default" (analyze.c:1065 with :1897). A zero means do not collect at all. + deftarget integer := current_setting('default_statistics_target')::integer; + nbuckets integer; + seen integer := 0; + disabled integer := 0; + unknown text; + schname text; + relnm text; BEGIN /* * Writing statistics uses pg_restore_attribute_stats, which core added in @@ -1059,12 +1078,32 @@ BEGIN END IF; FOR att IN - SELECT a.attname, a.attnum, a.atttypid + SELECT a.attname, a.attnum, a.atttypid, a.attstattarget FROM pg_attribute a WHERE a.attrelid = rel AND a.attnum > 0 AND NOT a.attisdropped AND (columns IS NULL OR a.attname = ANY (columns)) ORDER BY a.attnum LOOP + /* + * The per-column statistics target, which is core's rule and not the + * global setting: + * + * attstattarget = isnull ? -1 : DatumGetInt16(dat); analyze.c:1065 + * if (attstattarget == 0) return NULL; :1070 + * if (stats->attstattarget < 0) :1897 + * stats->attstattarget = default_statistics_target; + * + * Zero means the DBA turned this column off, and honouring it is not + * optional: writing statistics for such a column overrides an explicit + * instruction and hands the planner numbers somebody disabled. Reading + * the global default for every column, as this function did, ignored + * ALTER TABLE ... SET STATISTICS entirely. + */ + IF att.attstattarget = 0 THEN + disabled := disabled + 1; + CONTINUE; + 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 @@ -1121,6 +1160,67 @@ BEGIN ndstat := 0; END IF; + /* + * Whether this type can be ordered at all. Hoisted out of the histogram + * test below because the most-common-value list needs the same answer: + * both order by the column, and a type with no btree opclass has no + * histogram in core either. + */ + orderable := EXISTS (SELECT 1 FROM pg_catalog.pg_type t + JOIN pg_catalog.pg_opclass oc ON oc.opcintype = t.oid + JOIN pg_catalog.pg_am am ON am.oid = oc.opcmethod + WHERE t.oid = att.atttypid AND am.amname = 'btree'); + + /* + * most_common_vals and most_common_freqs (#414 slice 3b). + * + * The selection rule is core's, and reading a complete column removes + * most of it. analyze_mcv_list() opens with + * + * if (samplerows == totalrows || totalrows <= 1.0) + * return num_mcv; -- analyze.c:2995 + * + * so the entire significance filter -- a continuity-corrected Wald + * interval over a hypergeometric variance -- is skipped when the whole + * table was read. That machinery exists to judge whether a SAMPLE + * frequency can be trusted; we do not sample, so the question does not + * arise and core's own answer is to keep the list. What remains: + * + * only values appearing more than once are eligible analyze.c:2549 + * the top default_statistics_target of those, by count analyze.c:2552 + * frequency = count / TOTAL rows, nulls included analyze.c:2720 + * + * That last one is the one that fails quietly. Dividing by the non-null + * count instead scales every frequency by 1/(1-null_frac): still ordered, + * still summing to less than one, still plausible, and wrong everywhere + * the column has nulls. test/analyze_function.sh pins it with a fixture + * that is one-tenth null, so the two denominators cannot agree. + * + * HAVING count(*) > 1 also reproduces core's unique-column case without a + * branch: when nothing repeats the aggregate is empty, array_agg returns + * NULL, and no MCV list is written -- which is what core does at + * analyze.c:2588 when nmultiple is zero. + * + * array_agg(...)::text rather than string_agg builds the array literal + * through the type's own output function, so quoting, embedded commas and + * braces are correct for text columns instead of being hand-assembled. + */ + mcvvals := NULL; + mcvfreqs := NULL; + IF orderable THEN + 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; + END IF; + /* * histogram_bounds, whose ends are exact because the read is complete * (#414 slice 3). @@ -1137,33 +1237,62 @@ BEGIN * Only for types that can be ordered. A column with no btree ordering * has no histogram in core either, and ORDER BY would simply fail. * - * Skipped when the column holds fewer distinct values than buckets: core - * emits no histogram there because the most-common-value list already - * describes the column completely, and writing one anyway would be a - * shape core never produces. + * The most-common values are EXCLUDED, which core does at analyze.c:2744 + * and :2768-2799 by collapsing them out of the sorted array before + * building buckets. Keeping them in counts them twice in selectivity: + * eqsel takes the value's frequency from the MCV list, and the range + * estimators count it again inside whichever bucket holds it. Nothing + * raises -- the estimates are simply inflated for the values a skewed + * column repeats most, which is where estimates matter. + * + * The population and the bucket count therefore both shrink, and both + * have to. Core sizes the histogram from what is LEFT: + * + * num_hist = ndistinct - num_mcv; + * if (num_hist > num_bins) num_hist = num_bins + 1; + * if (num_hist >= 2) { ... } -- analyze.c:2744-2747 * - * NOTE, and it bounds what this slice claims: core EXCLUDES - * most-common-values from the histogram. This function does not write - * most-common-values at all, so there is nothing here to double-count - * and the two are consistent as written. Writing both without that - * exclusion would over-count those values in selectivity, which is why - * most_common_vals is a separate slice and not a line added here. + * so it emits between 2 and num_bins+1 bounds and none at all below two. + * Asking percentile_disc for a fixed default_statistics_target+1 + * fractions regardless would repeat values once the remaining population + * is smaller than that -- a 150-distinct column with 100 most-common + * values has 50 left and would get 101 bounds, most of them duplicates. + * A histogram with repeated bounds describes buckets holding no rows, + * which is a shape core never emits. */ + nmcv := coalesce(array_length(mcvfreqs, 1), 0); + nremaining := ndistinct - nmcv; + hist := NULL; IF att.attnum > 0 - AND EXISTS (SELECT 1 FROM pg_catalog.pg_type t - JOIN pg_catalog.pg_opclass oc ON oc.opcintype = t.oid - JOIN pg_catalog.pg_am am ON am.oid = oc.opcmethod - WHERE t.oid = att.atttypid AND am.amname = 'btree') - AND ndistinct > nbuckets + AND orderable + AND nremaining >= 2 THEN + /* + * least(nbuckets, nremaining - 1) fractions, so the bound count is + * least(nbuckets + 1, nremaining): core's cap, reached from below. + */ + nfrac := least(nbuckets, nremaining - 1); + + /* + * The exclusion is a literal list rather than a re-aggregation. The + * alternative -- recomputing the most-common set in a subquery -- is + * a third full pass over a column this function exists to read once, + * and it can disagree with the list actually written if the tie-break + * ever differs. format_type gives the element type without a typmod, + * which is what the array literal must be parsed against. + */ EXECUTE format( 'SELECT percentile_disc( (SELECT array_agg(i::double precision / %s ORDER BY i) FROM generate_series(0, %s) i)) WITHIN GROUP (ORDER BY %I)::text - FROM %I.%I WHERE %I IS NOT NULL', - nbuckets, nbuckets, att.attname, schname, relnm, att.attname) + FROM %I.%I WHERE %I IS NOT NULL %s', + nfrac, 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)) + END) INTO hist; END IF; @@ -1174,32 +1303,46 @@ BEGIN * must be real (the division yields double precision). Without these the * call "succeeds" having stored nothing. * - * histogram_bounds is passed as text, which is what the function takes: - * it parses the array literal against the column's own type. + * histogram_bounds and most_common_vals are passed as text, which is what + * the function takes (attribute_stats.c:70,72): it parses each array + * literal against the column's own type. most_common_freqs is real[] + * (:71) -- a float8[] there is dropped with a WARNING, not an error. + * + * One call with typed NULLs rather than a branch per combination. A NULL + * argument is not written: each statistic is gated on PG_ARGISNULL + * (:162-163 for the MCV pair), so a typed NULL and an omitted argument + * mean the same thing. Four optional statistics would otherwise be + * sixteen call sites. The NULLs must still be TYPED -- an untyped NULL + * reaches VARIADIC "any" as `unknown` and is the mistyped-argument case + * these casts exist to avoid. + * + * most_common_vals and most_common_freqs are a pair: supplying one + * without the other is a WARNING and drops both (stats_check_arg_pair, + * :265). They are computed together above, so they are null together. */ - IF hist IS NULL THEN - PERFORM pg_catalog.pg_restore_attribute_stats( - 'schemaname', schname, - 'relname', relnm, - 'attname', att.attname::text, - 'inherited', false, - 'null_frac', nullfrac::real, - 'n_distinct', ndstat::real); - ELSE - PERFORM pg_catalog.pg_restore_attribute_stats( - 'schemaname', schname, - 'relname', relnm, - 'attname', att.attname::text, - 'inherited', false, - 'null_frac', nullfrac::real, - 'n_distinct', ndstat::real, - 'histogram_bounds', hist); - END IF; + PERFORM pg_catalog.pg_restore_attribute_stats( + 'schemaname', schname, + 'relname', relnm, + 'attname', att.attname::text, + 'inherited', false, + 'null_frac', nullfrac::real, + 'n_distinct', ndstat::real, + 'most_common_vals', mcvvals::text, + 'most_common_freqs', mcvfreqs::real[], + 'histogram_bounds', hist::text); seen := seen + 1; END LOOP; - IF seen = 0 THEN + /* + * Collecting nothing is an error only when nothing ASKED us not to. A column + * at SET STATISTICS 0 is an instruction, and core does not raise for + * `ANALYZE t (col)` when col is disabled -- it collects nothing and returns. + * Without the second term this guard turned that instruction into an error + * whose hint blamed missing row groups, which is a different fault entirely + * and would send somebody looking at the storage. + */ + IF seen = 0 AND disabled = 0 THEN RAISE EXCEPTION 'pgcolumnar.analyze(): collected statistics for no columns of %', rel::text USING HINT = 'the table may have no written row groups yet'; END IF; diff --git a/test/analyze_differential.sh b/test/analyze_differential.sh new file mode 100755 index 0000000..a6de58a --- /dev/null +++ b/test/analyze_differential.sh @@ -0,0 +1,325 @@ +#!/usr/bin/env bash +# +# pgColumnar: what pgcolumnar.analyze() writes must have the SHAPE core writes +# (issue #414, the differential harness that lands with slice 3b). +# +# This suite exists because of one property of the API the function writes +# through. pg_restore_attribute_stats takes VARIADIC "any" and validates each +# argument's type at run time. A mistyped argument is not an error: +# +# if (!stats_check_arg_array(fcinfo, attarginfo, MOST_COMMON_FREQS_ARG)) +# { +# do_mcv = false; -- attribute_stats.c:247-251 +# result = false; +# } +# +# It emits a WARNING, sets the argument to NULL, and carries on. The call then +# returns cleanly having stored nothing. Nothing in the value-level suite catches +# that: test/analyze_function.sh asserts our numbers are exact, and a statistic +# that was never written simply leaves core's earlier numbers in place, so the +# assertions read core's work and report on ours. That is not hypothetical -- the +# first draft of the slice 3b checks did exactly this and scored three of four +# passes against a function that wrote no most-common values at all. +# +# most_common_freqs is real[] and most_common_vals is text (attribute_stats.c:70-71); +# they are also a PAIR, so supplying one without the other drops both (:265). +# float8[] instead of real[] is the easiest mistake to make and the hardest to +# see, because plpgsql will happily produce one. +# +# So this suite compares SHAPE against core rather than values. Values must +# differ -- ours are exact and core's are sampled, which is the whole feature, so +# values cannot be the oracle. Shape must not differ: for every statistic kind we +# write, core writing the same kind for the same column must agree on the +# operator, the collation, and the element type of the stored array. A dropped +# argument leaves the slot absent, which a shape comparison sees immediately. +# +# Core ANALYZE is therefore the oracle here, the same way heap is the oracle for +# columnar in test/differential.sh. +# +# Usage: test/analyze_differential.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +pgc_setup "${1:-/usr/local/pg18/bin/pg_config}" + +ROWS=${PGC_ANALYZE_DIFF_ROWS:-50000} + +# Same version gate as test/analyze_function.sh, and for the same reason: a major +# without pg_restore_attribute_stats is not a defect anybody can fix, so it is a +# SKIP with no checks (exit 66) rather than pgc_skip, which fails by default and +# would redden every PG15-17 run the moment this suite was registered. +# +# The major is asserted first so an unreadable version is not mistaken for an old +# one and reported as "supported, skipped". +if ! pgc_is_number "${PGC_MAJOR:-}"; then + echo "FAIL could not read the server major, so the gate below cannot be trusted: got [${PGC_MAJOR:-}]" + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + pgc_summary +fi +if [ "$PGC_MAJOR" -lt 18 ]; then + echo "SKIP pgcolumnar.analyze() needs pg_restore_attribute_stats (PG18+); this server is $PGC_MAJOR" + pgc_summary +fi + +# --- fixture ------------------------------------------------------------------ +# +# Five types, because the failure this suite hunts is type-dependent. An int +# column can be written correctly by code that mangles every text column: the +# array literal for most_common_vals is built through the type's own output +# function, and the two text values below are the ones that break a literal +# assembled by hand instead -- one contains a comma, the other a quote. +# +# Each column is skewed the same way: two values repeat heavily and the rest are +# unique, so core produces both a most-common list and a histogram and there is a +# shape to compare. `b` is deliberately different: two distinct values, both +# repeated, so the MCV list describes the column completely and NEITHER core nor +# this function emits a histogram (analyze.c:2744, num_hist = ndistinct - num_mcv). + +# Every column is one-in-seven NULL, and that is load-bearing rather than +# realistic. Frequencies are count / TOTAL rows including nulls (analyze.c:2720); +# dividing by the non-null count instead is the quiet defect this suite should +# catch. Written WITHOUT nulls -- as this fixture first was -- the two +# denominators are the same number, the frequency checks below cannot tell them +# apart, and the whole suite passed with that defect injected while +# test/analyze_function.sh caught it. A differential harness that misses the +# failure the value suite catches is not adding coverage, and the removal proof +# is what exposed it. + +psql_run "DROP TABLE IF EXISTS ad_c; + CREATE TABLE ad_c (i int, t text, n numeric, d date, b boolean) USING pgcolumnar; + INSERT INTO ad_c SELECT + CASE WHEN g % 7 = 0 THEN NULL + WHEN g % 5 = 0 THEN 7 WHEN g % 5 = 1 THEN 42 ELSE 1000 + g END, + CASE WHEN g % 7 = 0 THEN NULL + WHEN g % 5 = 0 THEN 'alpha,beta' WHEN g % 5 = 1 THEN 'it''s here' + ELSE 'v' || g END, + CASE WHEN g % 7 = 0 THEN NULL + WHEN g % 5 = 0 THEN 1.5 WHEN g % 5 = 1 THEN 2.25 + ELSE (1000 + g)::numeric END, + CASE WHEN g % 7 = 0 THEN NULL + WHEN g % 5 = 0 THEN DATE '2020-01-01' WHEN g % 5 = 1 THEN DATE '2021-06-15' + ELSE DATE '2000-01-01' + g END, + CASE WHEN g % 7 = 0 THEN NULL ELSE (g % 3 = 0) END + FROM generate_series(1, $ROWS) g;" >/dev/null + +check_num "premise: the fixture loaded, so the shapes below describe real data" \ + "$(q "SELECT count(*) FROM ad_c")" "$ROWS" + +# The premise that makes every frequency check below discriminating. If the +# column were never null, count/total and count/non-null would agree and a wrong +# denominator would pass unseen. +check "premise: the columns are nullable in fact, so the two denominators differ" \ + "$(q "SELECT CASE WHEN count(*) FILTER (WHERE i IS NULL) > 0 THEN 'yes' ELSE 'no' END + FROM ad_c")" "yes" + +# The text values that break a hand-built array literal are actually present. +# Without this, a green run could mean the quoting was never exercised. Counted +# by query rather than by arithmetic over ROWS, because the null pattern and the +# skew pattern overlap and the closed form is a distraction. +check "premise: a most-common text value contains a comma" \ + "$(if [ "$(q "SELECT count(*) FROM ad_c WHERE t = 'alpha,beta'")" -gt 0 ] 2>/dev/null; then echo yes; else echo no; fi)" "yes" +check "premise: and another contains a quote" \ + "$(if [ "$(q "SELECT count(*) FROM ad_c WHERE t = 'it''s here'")" -gt 0 ] 2>/dev/null; then echo yes; else echo no; fi)" "yes" + +# --- core's shape, which is the oracle ---------------------------------------- +# +# Captured into an ordinary table rather than a temp one: every psql_run and q() +# opens its own connection, so a temp table would not survive to be read. +# +# The five stakind/staop/stacoll/stavalues slots are unnested WITH ORDINALITY so +# each kind stays joined to the slot it was found in. Comparing kind sets alone +# would miss an operator or collation written into the right kind's slot but +# wrong, which is the silent half of this failure mode. + +psql_run "ANALYZE ad_c;" >/dev/null + +psql_run "DROP TABLE IF EXISTS ad_shape; + CREATE TABLE ad_shape (source text, attname name, kind smallint, + op oid, coll oid, elemtype text); + INSERT INTO ad_shape + SELECT 'core', a.attname, s.kind, s.op, s.coll, s.elemtype + FROM pg_attribute a + JOIN pg_statistic st ON st.starelid = a.attrelid AND st.staattnum = a.attnum + CROSS JOIN LATERAL ( + SELECT k.kind, o.op, c.coll, + CASE k.ord + WHEN 1 THEN pg_typeof(st.stavalues1)::text + WHEN 2 THEN pg_typeof(st.stavalues2)::text + WHEN 3 THEN pg_typeof(st.stavalues3)::text + WHEN 4 THEN pg_typeof(st.stavalues4)::text + WHEN 5 THEN pg_typeof(st.stavalues5)::text + END AS elemtype + FROM unnest(ARRAY[st.stakind1, st.stakind2, st.stakind3, st.stakind4, st.stakind5]) + WITH ORDINALITY AS k(kind, ord) + JOIN unnest(ARRAY[st.staop1, st.staop2, st.staop3, st.staop4, st.staop5]) + WITH ORDINALITY AS o(op, ord) ON o.ord = k.ord + JOIN unnest(ARRAY[st.stacoll1, st.stacoll2, st.stacoll3, st.stacoll4, st.stacoll5]) + WITH ORDINALITY AS c(coll, ord) ON c.ord = k.ord + ) s + WHERE a.attrelid = 'ad_c'::regclass AND a.attnum > 0 AND NOT a.attisdropped + AND NOT st.stainherit AND s.kind <> 0;" >/dev/null + +# STATISTIC_KIND_MCV is 1 and STATISTIC_KIND_HISTOGRAM is 2 (pg_statistic.h). +# Asserted rather than assumed: if core produced neither for these columns, every +# comparison below would compare an empty set with an empty set and pass. +check_num "premise: core produced a most-common list for every column" \ + "$(q "SELECT count(DISTINCT attname) FROM ad_shape WHERE source = 'core' AND kind = 1")" "5" +check_num "premise: and a histogram for the four with a tail, but not for boolean" \ + "$(q "SELECT count(DISTINCT attname) FROM ad_shape WHERE source = 'core' AND kind = 2")" "4" + +# --- clear, so what follows is unambiguously ours ------------------------------ +# +# pg_restore_attribute_stats leaves kinds it was not given in place, which is +# correct (test/analyze_function.sh pins it) and fatal to attribution here: a +# statistic we failed to write would still be present, wearing core's shape, and +# every check below would pass on core's work. + +psql_run "SELECT pg_catalog.pg_clear_attribute_stats('public', 'ad_c', a.attname::text, false) + FROM pg_attribute a + WHERE a.attrelid = 'ad_c'::regclass AND a.attnum > 0 AND NOT a.attisdropped;" >/dev/null + +check_num "premise: every statistic is gone before we write, so nothing below is core's" \ + "$(q "SELECT count(*) FROM pg_statistic WHERE starelid = 'ad_c'::regclass")" "0" + +# --- our call ------------------------------------------------------------------ + +ad_out="$(psql_run "SELECT pgcolumnar.analyze('ad_c'::regclass);" 2>&1)" +check_num "pgcolumnar.analyze() ran over every column without raising" \ + "$(grep -c 'ERROR' <<<"$ad_out")" "0" + +# The check this suite is named for. A WARNING here IS the silent wrong write: +# the argument was dropped, the call succeeded, and the statistic is missing. +check_num "and without a WARNING, which is how pg_restore_attribute_stats drops an argument" \ + "$(grep -c 'WARNING' <<<"$ad_out")" "0" + +psql_run "INSERT INTO ad_shape + SELECT 'ours', a.attname, s.kind, s.op, s.coll, s.elemtype + FROM pg_attribute a + JOIN pg_statistic st ON st.starelid = a.attrelid AND st.staattnum = a.attnum + CROSS JOIN LATERAL ( + SELECT k.kind, o.op, c.coll, + CASE k.ord + WHEN 1 THEN pg_typeof(st.stavalues1)::text + WHEN 2 THEN pg_typeof(st.stavalues2)::text + WHEN 3 THEN pg_typeof(st.stavalues3)::text + WHEN 4 THEN pg_typeof(st.stavalues4)::text + WHEN 5 THEN pg_typeof(st.stavalues5)::text + END AS elemtype + FROM unnest(ARRAY[st.stakind1, st.stakind2, st.stakind3, st.stakind4, st.stakind5]) + WITH ORDINALITY AS k(kind, ord) + JOIN unnest(ARRAY[st.staop1, st.staop2, st.staop3, st.staop4, st.staop5]) + WITH ORDINALITY AS o(op, ord) ON o.ord = k.ord + JOIN unnest(ARRAY[st.stacoll1, st.stacoll2, st.stacoll3, st.stacoll4, st.stacoll5]) + WITH ORDINALITY AS c(coll, ord) ON c.ord = k.ord + ) s + WHERE a.attrelid = 'ad_c'::regclass AND a.attnum > 0 AND NOT a.attisdropped + AND NOT st.stainherit AND s.kind <> 0;" >/dev/null + +# --- the differential ---------------------------------------------------------- +# +# Asserted first, because every comparison that follows is over the rows this +# counts. If the function wrote nothing, "no kind we wrote disagrees with core" +# is true of the empty set and the suite would report success for a function that +# does nothing at all -- the exact failure this file exists to detect. + +check_num "we wrote a most-common list for every column, as core did" \ + "$(q "SELECT count(DISTINCT attname) FROM ad_shape WHERE source = 'ours' AND kind = 1")" "5" + +check_num "and a histogram for exactly the four core gave one" \ + "$(q "SELECT count(DISTINCT attname) FROM ad_shape WHERE source = 'ours' AND kind = 2")" "4" + +# The columns must be the SAME four, not merely four of them. +check_num "and they are the same four columns, not just the same count" \ + "$(q "SELECT count(*) FROM ( + SELECT attname FROM ad_shape WHERE source = 'ours' AND kind = 2 + EXCEPT + SELECT attname FROM ad_shape WHERE source = 'core' AND kind = 2) t")" "0" + +# Every kind we wrote must exist in core's shape for that column carrying the +# same operator and collation. A missing counterpart counts as a mismatch, which +# is what the LEFT JOIN with an IS NULL test does. +check_num "every statistic we wrote agrees with core on operator and collation" \ + "$(q "SELECT count(*) FROM ad_shape o + LEFT JOIN ad_shape c ON c.source = 'core' AND c.attname = o.attname + AND c.kind = o.kind AND c.op = o.op AND c.coll = o.coll + WHERE o.source = 'ours' AND c.attname IS NULL")" "0" + +# The element type of the stored array is NOT checked here, and the reason is +# worth writing down because the check that was here looked right and measured +# nothing. +# +# pg_statistic.stavalues1 is declared `anyarray` (pg_statistic.h:119), so +# pg_typeof(stavalues1) returns the static type of the expression -- the constant +# string "anyarray" -- for every row ever stored. Comparing that against the +# column's type reported all nine of our slots as mismatched, which looked like a +# defect in the function and was a defect in the probe: it fails identically +# against CORE's own statistics, so it was testing the expectation rather than +# the code. A count that comes back equal to the total is a probe result, not a +# measurement. +# +# What IS observable is stronger anyway, and is asserted below: whether each +# stored value exists in the column with exactly the stored frequency. A value +# mangled by bad quoting, or a frequency scaled by the wrong denominator, fails +# that on the actual data rather than on a type name. + +# --- the values and frequencies mean what they say, per type ------------------- +# +# Run per column with the column's own type, because this is where a type- +# dependent defect surfaces: text literals lose their quoting, numeric and date +# have their own output forms. The oracle is an independent count over the table, +# never a re-reading of what the function wrote. +# +# A tolerance is required and is not slack: most_common_freqs is real (float4, +# ~7 significant digits) while the true frequency is exact numeric, so demanding +# equality would fail on representation rather than on correctness. + +for spec in "i int" "t text" "n numeric" "d date" "b boolean"; do + col="${spec%% *}" + typ="${spec#* }" + + # Asserted before the comparison: an empty MCV list makes "no value disagrees" + # true of nothing, which is the vacuous pass this whole suite is about. + nmcv="$(q "SELECT coalesce(array_length(most_common_vals::text::${typ}[], 1), 0) + FROM pg_stats WHERE tablename = 'ad_c' AND attname = '$col'")" + check "premise: $col has a most-common list to check" \ + "$(if pgc_is_number "$nmcv" && [ "$nmcv" -ge 1 ]; then echo yes; else echo "no (length [$nmcv])"; fi)" \ + "yes" + + check_num "every most-common value of $col exists with exactly its stored frequency" \ + "$(q "WITH m AS ( + SELECT unnest(most_common_vals::text::${typ}[]) AS v, + unnest(most_common_freqs) AS f + FROM pg_stats WHERE tablename = 'ad_c' AND attname = '$col') + SELECT count(*) FROM m + WHERE abs((SELECT count(*) FROM ad_c WHERE ad_c.$col IS NOT DISTINCT FROM m.v)::numeric + / $ROWS - m.f::numeric) > 0.000001")" \ + "0" +done + +# --- the values themselves survived the round trip ---------------------------- +# +# Shape agreement does not prove the text column's array literal was assembled +# correctly: a literal that lost a comma still parses, into the wrong values. +# These two are the ones built to break it. + +check_num "the most-common text value containing a comma round-tripped intact" \ + "$(q "SELECT count(*) FROM pg_stats + WHERE tablename = 'ad_c' AND attname = 't' + AND 'alpha,beta' = ANY (most_common_vals::text::text[])")" "1" + +check_num "and the one containing a quote" \ + "$(q "SELECT count(*) FROM pg_stats + WHERE tablename = 'ad_c' AND attname = 't' + AND 'it''s here' = ANY (most_common_vals::text::text[])")" "1" + +# boolean has two distinct values, both repeated, so the MCV list describes the +# column completely and there is nothing left to bucket. Core emits no histogram +# and neither may we: num_hist = ndistinct - num_mcv = 0 (analyze.c:2744). +check "boolean gets no histogram, because the most-common list already describes it" \ + "$(q "SELECT coalesce((SELECT histogram_bounds::text FROM pg_stats + WHERE tablename = 'ad_c' AND attname = 'b'), '')")" "" + +pgc_summary diff --git a/test/analyze_function.sh b/test/analyze_function.sh index c2af20e..05fcdf6 100755 --- a/test/analyze_function.sh +++ b/test/analyze_function.sh @@ -77,10 +77,15 @@ fi # sample is what makes the sampled estimate inexact, which slice 1 depends on. psql_run "DROP TABLE IF EXISTS af_c; - CREATE TABLE af_c (k int, skew int, pad1 text, pad2 text, pad3 text) USING pgcolumnar; + CREATE TABLE af_c (k int, skew int, cat int, pad1 text, pad2 text, pad3 text) USING pgcolumnar; INSERT INTO af_c SELECT CASE WHEN g % 10 = 0 THEN NULL ELSE g % 45001 END, CASE WHEN g = 1 THEN 1000000 ELSE g % 100000 END, + CASE WHEN g % 10 = 0 THEN NULL + WHEN g <= 100000 THEN 7 + WHEN g <= 160000 THEN 42 + WHEN g <= 190000 THEN 99 + ELSE 1000 + g END, md5(g::text), md5((g * 7)::text), md5((g * 13)::text) FROM generate_series(1, $ROWS) g;" >/dev/null @@ -274,10 +279,29 @@ check_num "pgcolumnar.analyze() puts the true maximum at the top of histogram_bo # Both ends, not just the interesting one. A histogram whose top is right and # whose bottom is invented is still wrong, and percentile_disc returning real # column values is what makes both exact. -check_num "and the true minimum at the bottom" \ +# +# The bottom is the smallest value that is NOT most-common, which is not the same +# as the column minimum and stopped being the same in slice 3b. `skew` is +# g % 100000, so 0 occurs five times, which makes it a most-common value and +# excludes it from the histogram (analyze.c:2744). This check read +# `SELECT min(skew)` while nothing was ever excluded and began failing with +# got [1] want [0] the moment the exclusion landed -- correctly, because 1 occurs +# four times rather than five and so misses the list that 0 makes. +# +# The expected value is derived from the written MCV list rather than hardcoded, +# so it stays right if the fixture or the bucket count moves. +check_num "and the smallest non-most-common value at the bottom" \ "$(q "SELECT (histogram_bounds::text::int[])[1] FROM pg_stats WHERE tablename = 'af_c' AND attname = 'skew'")" \ - "$(q "SELECT min(skew) FROM af_c")" + "$(q "WITH m AS (SELECT most_common_vals::text::int[] AS v FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'skew') + SELECT min(a.skew) FROM af_c a, m WHERE a.skew <> ALL (m.v)")" + +# The exclusion holds for skew too, not only for the column built to show it. +check_num "and no most-common value is inside skew's histogram either" \ + "$(q "SELECT count(*) FROM pg_stats s, unnest(s.histogram_bounds::text::int[]) b + WHERE s.tablename = 'af_c' AND s.attname = 'skew' + AND b = ANY (s.most_common_vals::text::int[])")" "0" # percentile_disc returns values the column HOLDS. percentile_cont would # interpolate and invent ones it does not, which is wrong for a histogram of @@ -306,4 +330,233 @@ check "and it is sorted ascending, which a histogram must be to be usable" \ THEN 'yes' ELSE 'no' END FROM pg_stats WHERE tablename = 'af_c' AND attname = 'skew'")" "yes" +# ---- slice 3b: most_common_vals and most_common_freqs ------------------------ +# +# The selection rule is core's, and it is not the sampled one. analyze_mcv_list() +# opens by refusing to filter at all when the whole table was read: +# +# /* +# * 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 machinery that guard skips -- a continuity-corrected Wald interval over a +# hypergeometric variance -- exists to decide whether a SAMPLE frequency is +# trustworthy enough to store. We read the entire column, so that question does +# not arise and core's own answer is "keep them". What is left is mechanical: +# +# only values with count > 1 are eligible analyze.c:2549 +# top default_statistics_target by count analyze.c:2552-2564 +# frequency is count / total rows INCLUDING nulls analyze.c:2720 +# +# `cat` is built so that the third of those is observable, because it is the one +# that fails silently. Three values repeat and nothing else does: +# +# NULL 50,000 one row in ten +# 7 90,000 freq 0.18 -- 0.2 if divided by the non-null count +# 42 54,000 freq 0.108 -- 0.12 if divided by the non-null count +# 99 27,000 freq 0.054 -- 0.06 if divided by the non-null count +# tail 279,000 distinct values, each appearing exactly once +# +# Dividing by the 450,000 non-null rows rather than the 500,000 total inflates +# every frequency by 1/(1-null_frac) and produces 0.2/0.12/0.06: three numbers +# that are individually plausible, sum to less than one, and are wrong. No error +# is raised on that path, so the fixture has to be the thing that catches it. +# That is why the null fraction is not zero here. +# +# Exactly three values repeat, so the list is fully determined rather than a +# top-N cut of a longer one, and the check can name it outright. + +cat_total="$(q "SELECT count(*) FROM af_c")" +check_num "premise: the MCV fixture has the row count the frequencies divide by" \ + "$cat_total" "$ROWS" + +check_num "premise: value 7 appears exactly 90,000 times" \ + "$(q "SELECT count(*) FROM af_c WHERE cat = 7")" "90000" +check_num "premise: value 42 appears exactly 54,000 times" \ + "$(q "SELECT count(*) FROM af_c WHERE cat = 42")" "54000" +check_num "premise: value 99 appears exactly 27,000 times" \ + "$(q "SELECT count(*) FROM af_c WHERE cat = 99")" "27000" + +# The rule at analyze.c:2549 is "count > 1", so this premise is what makes the +# expected list exactly three long. If the tail ever stopped being unique, the +# list would fill to default_statistics_target with tied values and the check +# below would fail for a reason that is not a defect. +check_num "premise: and nothing else in the column repeats, so the list is exactly three" \ + "$(q "SELECT count(*) FROM (SELECT cat FROM af_c WHERE cat IS NOT NULL + GROUP BY cat HAVING count(*) > 1) t")" "3" + +# Core's sampled frequencies, captured before our call overwrites them. +psql_run "ANALYZE af_c;" >/dev/null +core_mcv_before="$(q "SELECT most_common_vals::text FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" +core_freq_7="$(q "SELECT (most_common_freqs)[array_position(most_common_vals::text::int[], 7)] + FROM pg_stats WHERE tablename = 'af_c' AND attname = 'cat'")" +echo "-- core sampled MCVs = ${core_mcv_before:-}, its freq for 7 = ${core_freq_7:-}" + +# Reported, not gated. Core's sample finding 7 at exactly 0.18 is unlikely but it +# is a sample, and a check that depends on core being unlucky is the shape slice 3 +# already had to retract. Exactness below is asserted against independent counts, +# so it does not need core to be wrong. +if pgc_is_number "$core_freq_7" && [ "$core_freq_7" != "0.18" ]; then + echo "-- core's sampled frequency is inexact, which is the gap this slice closes" +fi + +# The statistics are CLEARED before our call, and this is not tidiness. +# +# Written without it, every check below read core's leftover MCV list and none of +# them could tell "our function wrote this" from "core wrote it and our function +# left it alone". Two passed that way on the first run: most_common_vals matched +# because core had already put {7,42,99} there, and 99's frequency matched because +# core's sample happened to round to 0.054 at six places. A function that writes +# no MCVs at all scored three of four. +# +# Clearing first makes attribution structural rather than lucky: after this call +# the column has no MCV list, so anything the checks below find is ours. +psql_run "SELECT pg_catalog.pg_clear_attribute_stats('public', 'af_c', 'cat', false);" >/dev/null +# A scalar subquery, because clearing removes the whole pg_statistic row rather +# than nulling a column: read as `SELECT ... FROM pg_stats WHERE ...` this returns +# NO ROWS, q() yields the empty string, and coalesce never runs. The premise then +# fails for its own reason rather than reporting the state it was asked about. +check "premise: the MCV list really is gone before we write, so what follows is ours" \ + "$(q "SELECT coalesce((SELECT most_common_vals::text FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'), '')")" \ + "" + +mcv_out="$(psql_run "SELECT pgcolumnar.analyze('af_c'::regclass, ARRAY['cat']);" 2>&1)" +check_num "pgcolumnar.analyze() ran without raising for the MCV column" \ + "$(grep -c 'ERROR' <<<"$mcv_out")" "0" + +# A WARNING here is the silent-wrong-write this slice was told to guard against. +# pg_restore_attribute_stats takes VARIADIC "any": most_common_vals must be text +# and most_common_freqs must be real[] (attribute_stats.c:70-71). A float8[] is +# not an error, it is a WARNING and a dropped argument, and the call then reports +# success having stored nothing. +check_num "and without a WARNING, which is how a mistyped argument is dropped" \ + "$(grep -c 'WARNING' <<<"$mcv_out")" "0" + +check "pgcolumnar.analyze() writes the three repeated values as most_common_vals" \ + "$(q "SELECT most_common_vals::text FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" \ + "{7,42,99}" + +# Each frequency against its own independently counted truth, not against a +# recomputation of what the function did. +check_num "and 7's frequency exactly, over total rows rather than non-null rows" \ + "$(q "SELECT round((most_common_freqs)[1]::numeric, 6) FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" \ + "$(q "SELECT round(90000::numeric / $ROWS, 6)")" + +check_num "and 42's" \ + "$(q "SELECT round((most_common_freqs)[2]::numeric, 6) FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" \ + "$(q "SELECT round(54000::numeric / $ROWS, 6)")" + +check_num "and 99's" \ + "$(q "SELECT round((most_common_freqs)[3]::numeric, 6) FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" \ + "$(q "SELECT round(27000::numeric / $ROWS, 6)")" + +# ---- the exclusion, which is why 3b could not be a line added to slice 3 ------ +# +# Core builds the histogram from the values left AFTER the most-common ones are +# removed (analyze.c:2744 num_hist = ndistinct - num_mcv, and the collapse loop at +# :2768-2799). Writing both lists without that exclusion counts those values +# twice in selectivity: eqsel finds the value in the MCV list and takes its +# frequency, and the range estimators count it again inside whichever bucket +# holds it. Nothing errors; the estimates are just wrong, and wrong in the +# direction that says a heavily-repeated value is more common than it is. +# +# This is the check that has to fail before the exclusion exists. `cat`'s three +# most-common values are 7, 42 and 99 -- the three SMALLEST values in the column, +# so an unexcluded histogram puts 7 at the bottom bound and the check below finds +# it immediately. + +# Asserted first, because the exclusion check is vacuously true when there is no +# histogram at all. "No MCV appears in histogram_bounds" passes trivially against +# NULL, so a change that silently stopped emitting histograms would read as a fix. +check "premise: a histogram exists for cat, so the exclusion below is not vacuous" \ + "$(q "SELECT CASE WHEN coalesce(array_length(histogram_bounds::text::int[], 1), 0) >= 2 + THEN 'yes' ELSE 'no' END + FROM pg_stats WHERE tablename = 'af_c' AND attname = 'cat'")" "yes" + +check_num "no most-common value appears in histogram_bounds, which would double-count it" \ + "$(q "SELECT count(*) FROM pg_stats s, unnest(s.histogram_bounds::text::int[]) b + WHERE s.tablename = 'af_c' AND s.attname = 'cat' + AND b = ANY (s.most_common_vals::text::int[])")" "0" + +# The same fact from the other side, and the one that shows the histogram is over +# the remaining population rather than merely filtered at the ends: the lowest +# bound must be the smallest value that is NOT most-common, not the column's +# minimum. Truth comes from an independent query, not from the function. +check_num "so the bottom bound is the smallest non-most-common value, not the column minimum" \ + "$(q "SELECT (histogram_bounds::text::int[])[1] FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'cat'")" \ + "$(q "SELECT min(cat) FROM af_c WHERE cat NOT IN (7, 42, 99)")" + +# ---- the per-column statistics target, which core reads and we did not -------- +# +# Core sizes both lists from the COLUMN's attstattarget, not from the global +# default_statistics_target: +# +# attstattarget = isnull ? -1 : DatumGetInt16(dat); -- analyze.c:1065 +# if (attstattarget == 0) return NULL; -- :1070, skip entirely +# if (stats->attstattarget < 0) -- :1897 +# stats->attstattarget = default_statistics_target; +# +# So NULL means "use the default", a positive value overrides it, and zero means +# do not collect statistics for this column at all. This function read the global +# setting for every column, which silently ignored ALTER TABLE ... SET STATISTICS. +# This suite has been setting it on `skew` since slice 3 while asserting nothing +# about it, so the divergence was already present here and invisible. +# +# skew is at SET STATISTICS 10, set above for core's benefit. Ten buckets means +# eleven bounds: percentile_disc is asked for target+1 fractions, which is the +# shape core caps at num_bins+1 (analyze.c:2746). + +check_num "premise: skew really is at a non-default statistics target" \ + "$(q "SELECT attstattarget FROM pg_attribute + WHERE attrelid = 'af_c'::regclass AND attname = 'skew'")" "10" + +check_num "premise: and the global default differs from it, so the two are distinguishable" \ + "$(q "SHOW default_statistics_target")" "100" + +# Cleared and re-run, which this check needs and did not originally have. The MCV +# section above calls plain ANALYZE to capture core's sampled list for `cat`, and +# that analyses EVERY column of af_c, skew included. Reading skew's histogram +# after it therefore reads CORE's -- and core honours attstattarget, so the check +# passed at eleven bounds while the function under test was still producing a +# hundred and one. Measured directly on an isolated table to find it. +psql_run "SELECT pg_catalog.pg_clear_attribute_stats('public', 'af_c', 'skew', false);" >/dev/null +psql_run "SELECT pgcolumnar.analyze('af_c'::regclass, ARRAY['skew']);" >/dev/null + +check_num "the histogram honours the column's statistics target, not the global default" \ + "$(q "SELECT array_length(histogram_bounds::text::int[], 1) FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'skew'")" "11" + +# The most-common list is sized by the same target, and by the same rule +# (analyze.c:2552 tracks at most attstattarget entries). +check "and so does the most-common list, which is capped by the same target" \ + "$(if [ "$(q "SELECT array_length(most_common_vals::text::int[], 1) FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'skew'")" -le 10 ] 2>/dev/null; then echo yes + else echo "no (length [$(q "SELECT array_length(most_common_vals::text::int[], 1) FROM pg_stats WHERE tablename = 'af_c' AND attname = 'skew'")])"; fi)" \ + "yes" + +# Zero is not "a small target", it is "do not collect". A column set to zero that +# comes back with statistics has had the DBA's instruction overridden, and the +# planner is then using numbers somebody deliberately turned off. +psql_run "ALTER TABLE af_c ALTER COLUMN pad1 SET STATISTICS 0;" >/dev/null +psql_run "SELECT pg_catalog.pg_clear_attribute_stats('public', 'af_c', 'pad1', false);" >/dev/null + +pad_out="$(psql_run "SELECT pgcolumnar.analyze('af_c'::regclass, ARRAY['pad1']);" 2>&1)" +check_num "pgcolumnar.analyze() ran for the zero-target column without raising" \ + "$(grep -c 'ERROR' <<<"$pad_out")" "0" + +check "a column at SET STATISTICS 0 is left alone, because that is what zero means" \ + "$(q "SELECT coalesce((SELECT 'wrote-' || attname FROM pg_stats + WHERE tablename = 'af_c' AND attname = 'pad1'), '')")" \ + "" + pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index a546ba7..e814d44 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -41,6 +41,7 @@ set -uo pipefail SUITES=( advisory_lock_class alter_column_type + analyze_differential analyze_function analyze_reltuples analyze_stats From 1e654ee82c8fbd8b24f9fc7640510452ceee86c0 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 7 Aug 2026 08:21:47 -0600 Subject: [PATCH 2/2] fix: the registration check lost a race with its own pipe, not a fork (#469) harness_selftest reported "unregistered: parquet_nested_import" on #476's PG18 CI, in a run whose own summary listed that suite as having PASSED. The two are not in conflict; the check was wrong. listed_suites | grep -qx "$name" || unregistered="$unregistered $name" This file runs under `set -o pipefail`. `grep -q` returns the moment it matches, which closes 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 then recorded as unregistered. The CI log carries both halves: "printf: write error: Broken pipe" from line 209, and the false red. The note above the cache blamed transient fork failures for this symptom in the #473 matrix -- innocent suites, different names each run, some majors only. That diagnosis is wrong, or at least incomplete: caching removed the forks and the symptom survived. A race between grep exiting and printf finishing explains every observed property, including why it never reproduces locally. Measured rather than argued: 4,000 names, matching the first, 200 attempts. With pipefail, 18 false negatives; without it, 0. At the real list size of 132 short names it does NOT reproduce locally -- the whole list fits the pipe buffer, so printf's single write completes before grep can exit -- which is consistent with it having been seen once, under CI load, on a name near the end of the list. The fix is to stop piping. Membership is now a case over the cached string, with newlines on both sides for the whole-line match grep -x provided. No reader, no EPIPE, no race at any list size. Three controls come with it, because a matcher that always matched would also have made the red go away while making the check meaningless: a registered name must be found, an absent one must not be, and a prefix of a registered name must not count as registered. Proven by removal: deleting analyze_differential from SUITES fails the check with that name. 43 checks green on PG18 and PG19. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013sY1RQR5MjZNUixA2um31r --- test/harness_selftest.sh | 59 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 981d1cd..303041e 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -198,6 +198,28 @@ not_a_suite() { # with four names each, different names each time, while PG15/18/19 passed. An # intermittent red naming innocent suites is the worst kind, so the premise below # makes an empty answer say what it is. +# +# THAT DIAGNOSIS WAS WRONG, or at best incomplete, and the caching did not cure +# the symptom it was written for. The real cause is this file's own `set -o +# pipefail` meeting a reader that exits early: +# +# listed_suites | grep -qx "$name" +# +# `grep -q` returns the moment it matches, which closes the pipe while printf is +# still writing. printf then takes EPIPE and exits non-zero, and under pipefail +# the PIPELINE reports that failure even though grep matched -- so a registered +# suite is recorded as unregistered. It is a race between grep exiting and printf +# finishing, which is why it never reproduces locally, why it names innocent +# suites, and why it names DIFFERENT ones each run. +# +# Measured directly rather than reasoned about: 4,000 names, matching the first, +# 200 attempts. With pipefail, 18 false negatives. Without it, 0. It surfaced +# again on #476's CI (PG18) with "unregistered: parquet_nested_import" beside a +# "printf: write error: Broken pipe" from line 209, on a run whose own summary +# listed that suite as having passed. +# +# The fix is to stop piping. The membership test below is a case over the cached +# string, which cannot lose a race it no longer runs. _SUITE_LIST="$(bash "$RUNNER" --list-suites 2>/dev/null)" check "premise: the runner answered --list-suites, so the two checks below mean something" \ "$([ -n "$_SUITE_LIST" ] && echo yes || echo "no (empty)")" "yes" @@ -288,11 +310,46 @@ _sorted_actual="$(listed_suites)" check "the suite list is sorted, so two new suites land in different places" \ "$([ "$_sorted_actual" = "$_sorted_expected" ] && echo sorted || echo "not sorted")" "sorted" +# A case over the cached list rather than `listed_suites | grep -qx`. The pipe +# was the defect: grep -q returns on its match, printf takes EPIPE, and pipefail +# turns that into a failed pipeline for a suite that IS registered. See the note +# above line 201. Newlines around both sides make it a whole-line match, which is +# what grep -x provided and what keeps a name from matching inside another. +# Both directions first, because a membership test that always matched would make +# the check below pass for every suite including genuinely unregistered ones -- +# which is the same green-by-construction failure the pipe version produced in +# reverse. The replacement has to be shown to answer, not merely to stop failing. +case $'\n'"$_SUITE_LIST"$'\n' in + *$'\n'isolation$'\n'*) _ctl_present=present ;; + *) _ctl_present=absent ;; +esac +check "positive control: the membership test finds a name that is registered" \ + "$_ctl_present" "present" + +case $'\n'"$_SUITE_LIST"$'\n' in + *$'\n'no_such_suite_exists$'\n'*) _ctl_absent=present ;; + *) _ctl_absent=absent ;; +esac +check "negative control: and does not find one that is not" \ + "$_ctl_absent" "absent" + +# A partial name must not match a whole entry, which is what grep -x guaranteed +# and what the surrounding newlines preserve. +case $'\n'"$_SUITE_LIST"$'\n' in + *$'\n'isolatio$'\n'*) _ctl_partial=present ;; + *) _ctl_partial=absent ;; +esac +check "and a prefix of a registered name is not treated as registered" \ + "$_ctl_partial" "absent" + unregistered="" for f in "$TESTDIR"/*.sh; do name="$(basename "$f" .sh)" not_a_suite "$name" && continue - listed_suites | grep -qx "$name" || unregistered="$unregistered $name" + case $'\n'"$_SUITE_LIST"$'\n' in + *$'\n'"$name"$'\n'*) ;; + *) unregistered="$unregistered $name" ;; + esac done check "every suite is registered in run_all_versions.sh" \ "$([ -z "$unregistered" ] && echo none || echo "unregistered:$unregistered")" "none"