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/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" 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