diff --git a/test/lib.sh b/test/lib.sh index 231fb05d..e9eb366b 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -480,6 +480,26 @@ check_timing() { check "$name" "$got" "$want" } +# A ratio check whose subject is a wall-clock ratio. +# +# check_timing does this for a scalar; a ratio needs its own entry point because +# check_ratio takes a bound as well as two sides. +# +# It exists so that a suite never has to read PGC_SKIP_TIMING to decide whether +# to ASSERT. planner_choice_quality did read it, branched on it, and then called +# check_timing with two empty strings for got and want -- the "" vs "" compare +# check_text and check_num were added to forbid (#418). That was safe only while +# the suite's copy of the condition agreed with this file's, which is exactly the +# coupling this helper removes. Deciding whether to MEASURE is still the suite's +# business; deciding whether to assert is this file's. +check_ratio_timing() { # check_ratio_timing + if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then + echo "SKIP $1 (PGC_SKIP_TIMING: wall-clock ratio)" + return 0 + fi + check_ratio "$@" +} + # Is this query's plan the columnar custom scan? # # For a check that reads a counter out of EXPLAIN and asserts on it. Those diff --git a/test/planner_choice_quality.sh b/test/planner_choice_quality.sh new file mode 100755 index 00000000..327dc2b2 --- /dev/null +++ b/test/planner_choice_quality.sh @@ -0,0 +1,226 @@ +#!/usr/bin/env bash +# +# The planner's chosen plan must not be catastrophically worse than one it +# declined (issues #433, #434). +# +# Every other suite asks whether a plan is CORRECT. None asks whether it is the +# one a reasonable cost model would pick. That gap is how #434 survived: the +# planner prices a columnar index scan at 31,502 and the custom scan at 69,204, +# then chooses the index scan, which is 12.4x slower. Both plans return the right +# answer, so every existing check passes. +# +# WHAT THIS ASSERTS +# +# For each query: run the planner's own choice, then force each alternative, and +# fail when the choice is more than PLAN_BOUND times slower than the best +# alternative. It is a ratio between two plans in the same run on the same box, +# so it does not depend on how fast the machine is. +# +# WHY THE BOUND IS LOOSE +# +# 3x. The point is not to police the cost model, which is allowed to be wrong. +# It is to catch the case where it is wrong by orders of magnitude, which is what +# #434 is. A tight bound here would flake on a shared runner and teach people to +# ignore it. +# +# WHY TIMING RATHER THAN BUFFERS +# +# Buffers would be exact, and they are the right tool when the question is "did +# this read less". Here the question is "is the chosen plan much slower", and +# slower is what the user experiences. The bound is loose enough to survive +# timing noise. +# +# PGC_SKIP_TIMING +# +# The ratio goes through check_ratio_timing, which lives in lib.sh and owns the +# decision. This file does NOT branch on the flag to decide whether to assert. +# +# Two earlier versions got this wrong in the same way. The first only SAID the +# flag was wired and used check_ratio throughout, so it did nothing. The second +# branched on the flag here and called check_timing with two empty strings, which +# is the "" vs "" compare lib.sh forbids, and which would have PASSED the suite's +# central assertion had the two copies of the condition ever disagreed. +# +# Whether to MEASURE is still this file's business, because that is a cost +# decision rather than an assertion: PGC_MEASURING below gates both the fixture +# size and the execution of the timed queries. +# +# Usage: test/planner_choice_quality.sh [PG_CONFIG] +# Written fresh for pgColumnar. +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +PLAN_BOUND=${PGC_PLAN_BOUND:-3} +# The only read of PGC_SKIP_TIMING in this file, and it governs COST alone: how +# big a fixture to build, and whether to execute the timed queries. Whether to +# assert a ratio is check_ratio_timing's decision, in lib.sh. +PGC_MEASURING=1 +[ "${PGC_SKIP_TIMING:-0}" = 1 ] && PGC_MEASURING=0 +ROWS=${PGC_PLAN_ROWS:-200000} +# 200,000 wide rows is 12.8M md5() calls and ~205 MB, and none of it is needed to +# read an EXPLAIN. When only the plan shapes are asserted, build a fixture sized +# for that. Shortening the statement timeout did not address this cost; the +# review asked about the suite's cost per major and this is where it lives. +[ "$PGC_MEASURING" = 1 ] || ROWS=${PGC_PLAN_ROWS:-20000} +# The finding is "23x slower", not "slower than two minutes". A long timeout only +# buys a longer wait before the same verdict, on every major, for as long as the +# bug exists. +PLAN_TIMEOUT=${PGC_PLAN_TIMEOUT:-20} + + +# The shape #433 and #434 are about: wide incompressible rows, an index on a +# correlated key. The payload must not compress, or the row group stays under the +# fetch cache cap and the effect disappears. Correlating on both g and the inner +# series is what makes every row and every block differ. +psql_run "CREATE TABLE pq (k bigint, tag text, payload bytea) USING pgcolumnar; + INSERT INTO pq + SELECT g, 'tag' || (g % 5), + decode((SELECT string_agg(md5(g::text || s::text), '') + FROM generate_series(1,64) s), 'hex') + FROM generate_series(1,$ROWS) g; + CREATE INDEX pq_k ON pq (k); + ANALYZE pq;" >/dev/null + +check "fixture rows" "$(q 'SELECT count(*) FROM pq')" "$ROWS" +SZ=$(q "SELECT pg_total_relation_size('pq')") +check "premise: the payload did not compress, so the fetch path is exercised" \ + "$([ "$SZ" -gt $(( ROWS * 700 )) ] && echo yes || echo "no ($(( SZ / ROWS )) bytes per row)")" "yes" +check "premise: the index key is correlated, which is the case that misprices" \ + "$(q "SELECT CASE WHEN correlation > 0.9 THEN 'yes' ELSE 'no (' || correlation || ')' END + FROM pg_stats WHERE tablename='pq' AND attname='k'")" "yes" + +# Time a query under a given setting, and report which scan node ran. +run_plan() { # run_plan -> " " + local node ms out + node=$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -Atq -c "$1" -c "EXPLAIN (COSTS OFF) $2" 2>&1 | + grep -oE 'Index Only Scan|Index Scan|Bitmap Heap Scan|Custom Scan \([A-Za-z]+\)|Seq Scan' | head -1) + # No execution under PGC_SKIP_TIMING. The plan name above comes from EXPLAIN, + # so every premise still holds; only the wall clock is unavailable. + if [ "$PGC_MEASURING" = 0 ]; then + printf '%s\t\n' "${node:-unknown}" + return + fi + # Warm first, then measure. The three plans are timed in a fixed order, so + # without this the chosen plan pays a cold cache and both alternatives run + # warm against a 3x bound -- the ordering alone could push a healthy ratio + # over it. The discarded run also populates shared buffers for the timed one. + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -Atq -c "SET statement_timeout='${PLAN_TIMEOUT}s';" -c "$1" -c "$2" >/dev/null 2>&1 + out=$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -Atq -c "SET statement_timeout='${PLAN_TIMEOUT}s';" -c "$1" -c '\timing on' -c "$2" 2>&1) + if grep -qiE 'timeout|canceling' <<<"$out"; then + printf '%s\tTIMEOUT\n' "${node:-unknown}"; return + fi + ms=$(grep -oE 'Time: [0-9.]+ ms' <<<"$out" | tail -1 | grep -oE '[0-9.]+') + # Tab-delimited, because a node name contains spaces: "Custom Scan + # (PgColumnarScan)" read back through a space-split gives node="Custom" and + # ms="Scan (PgColumnarScan) 23.380". The first version of this file did that + # and compared two garbage strings while reporting a pass. + printf '%s\t%s\n' "${node:-unknown}" "${ms:-}" +} + +# The planner's choice against the best alternative it declined. +choice_vs_best() { # choice_vs_best