diff --git a/bench/cb_guards.sh b/bench/cb_guards.sh new file mode 100644 index 0000000..511d841 --- /dev/null +++ b/bench/cb_guards.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# +# Guards for the ClickBench harness (#465). +# +# Separate from run_clickbench.sh so they can be sourced and tested without the +# 15 GB download and the tuned cluster the benchmark needs. test/bench_guards.sh +# runs them in the ordinary matrix; the benchmark itself cannot. +# +# Sourced, not executed. Nothing here touches a database or the filesystem. +# Written fresh for pgColumnar. + +# cb_prepared_xacts_ok +# +# pgcolumnar.parallel_copy prepares one transaction per worker, so the cluster +# needs at least that many slots. The stock max_prepared_transactions is 0, which +# means every parallel arm errors out on its first worker. +# +# This must be asked BEFORE any arm is loaded, because the setting is +# PGC_POSTMASTER: raising it needs a restart, and discovering it mid-run wastes +# the whole run. +# +# A serial arm asks for 0 workers and needs no slots, so it is never blocked by a +# setting it does not use. +cb_prepared_xacts_ok() { + local current="${1:-}" workers="${2:-}" + case "$current" in '' | *[!0-9]*) return 1 ;; esac + case "$workers" in '' | *[!0-9]*) return 1 ;; esac + [ "$current" -ge "$workers" ] +} + +# cb_prepared_xacts_message +# +# The message is the deliverable. Without it the operator meets a per-worker +# error in a load log and has to work back to the cause; with it they are told +# the setting, the value, and that it costs a restart. +cb_prepared_xacts_message() { + local current="${1:-}" workers="${2:-}" + printf '%s\n' \ + "max_prepared_transactions is ${current:-unset}, and a ${workers}-worker parallel arm needs ${workers}." \ + "pgcolumnar.parallel_copy prepares one transaction per worker, so every arm would fail at once." \ + "Set max_prepared_transactions = ${workers} (or more) and restart the postmaster; it cannot be changed in a session." +} + +# cb_rows_ok +# +# A load that lost rows is a failure and not a fast result. An errored parallel +# arm leaves an EMPTY table and returns in about no time, which reads as perfect +# scaling; #465 records a harness that published exactly that. +# +# Both sides must be numbers. A psql that failed yields an empty string, and +# comparing "" with "" passes while measuring nothing, which is the trap #418 +# exists to forbid. +cb_rows_ok() { + local got="${1:-}" want="${2:-}" + case "$got" in '' | *[!0-9]*) return 1 ;; esac + case "$want" in '' | *[!0-9]*) return 1 ;; esac + [ "$got" = "$want" ] +} diff --git a/bench/run_clickbench.sh b/bench/run_clickbench.sh index fd6b656..3b0161d 100755 --- a/bench/run_clickbench.sh +++ b/bench/run_clickbench.sh @@ -115,6 +115,15 @@ CB_PORT="${PGC_CB_PORT:-58900}" CB_PGDATA="${PGC_CB_PGDATA:-$CB_DATA/pgdata}" CB_KEEP="${PGC_CB_KEEP:-0}" CB_MAXGROUPS="${PGC_CB_MAXGROUPS:-200000000}" +# Workers for the parallel-load arm. 0 disables the arm entirely; the serial arm +# is never affected. Reported ALONGSIDE the serial number, not instead of it: +# serial COPY is the single-connection ingest path and parallel_copy is the bulk +# path, and both are worth publishing (#465). +CB_PCOPY_WORKERS="${PGC_CB_PCOPY_WORKERS:-16}" + +# The load guards live in their own file so the matrix can test them without the +# 15 GB download this script needs. See test/bench_guards.sh. +. "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/cb_guards.sh" CB_URL_BASE="https://raw.githubusercontent.com/ClickHouse/ClickBench/main/postgresql" CB_TSV_URL="https://datasets.clickhouse.com/hits_compatible/hits.tsv.gz" @@ -304,6 +313,10 @@ max_worker_processes = $(( NCPU + 15 )) max_parallel_workers = $NCPU max_parallel_workers_per_gather = $(( NCPU / 2 )) max_parallel_maintenance_workers = $(( NCPU / 2 )) +# One prepared transaction per parallel_copy worker. The stock value is 0, which +# makes every parallel arm fail on its first worker; the preflight below refuses +# to start rather than let that be reported as a fast load (#465). +max_prepared_transactions = $CB_PCOPY_WORKERS listen_addresses = '' unix_socket_directories = '/tmp' CONF @@ -328,6 +341,22 @@ EXTVER=$($PSQL -At -c "SELECT extversion FROM pg_extension WHERE extname='pgcolu require "the extension is installed" "$([ -n "$EXTVER" ] && echo yes || echo no)" "yes" || exit 1 note " pgcolumnar $EXTVER on port $CB_PORT" +# ---- preflight the parallel arm, before any row is loaded (#465) ------------ +# +# max_prepared_transactions is PGC_POSTMASTER. Discovering it is too low during +# the load means the run is already wasted, and the failure arrives as a +# per-worker error in a load log rather than as its cause. So ask the RUNNING +# cluster, not the file this script wrote, because an operator may be pointing it +# at a cluster they tuned themselves. +if [ "$CB_PCOPY_WORKERS" -gt 0 ]; then + CB_PREPARED=$($PSQL -At -c "SHOW max_prepared_transactions") + if ! cb_prepared_xacts_ok "$CB_PREPARED" "$CB_PCOPY_WORKERS"; then + cb_prepared_xacts_message "$CB_PREPARED" "$CB_PCOPY_WORKERS" >&2 + die "the parallel arm cannot run; set PGC_CB_PCOPY_WORKERS=0 to skip it" + fi + note " parallel arm: $CB_PCOPY_WORKERS workers, max_prepared_transactions=$CB_PREPARED" +fi + # --------------------------------------------------------------------------- # 4. One table per arm, from the same fetched DDL # --------------------------------------------------------------------------- @@ -425,6 +454,54 @@ if printf '%s\n' "${ARMS[@]}" | grep -qx duckdb; then note " duckdb: ${LOAD_S[duckdb]}s, ${ROWS[duckdb]} rows, ${SIZE_B[duckdb]} bytes" fi +# ---- the parallel load arm, reported beside the serial one (#465) ---------- +# +# Serial COPY is the single-connection ingest path; pgcolumnar.parallel_copy is +# the bulk path. They measure different capabilities and both are published, so +# this is an EXTRA table rather than a replacement for the serial number. +# +# It is deliberately not in ARMS: the 43 queries have nothing to say about a +# table that exists only to be loaded, and running them would double the run for +# no reading. That also means it needs its own row assertion, below, rather than +# inheriting the one over ARMS. +# +# The load must fail LOUDLY. With max_prepared_transactions too low every worker +# errors at once and the load returns in about no time, which reads as perfect +# scaling: #465 records a harness that published 0.0s / 0.8s / 1.1s / 1.3s and +# meant "every arm failed". The preflight above stops that before it starts, and +# ON_ERROR_STOP plus the row check below stop it if it happens anyway. +if [ "$CB_PCOPY_WORKERS" -gt 0 ] && [ -n "${ROWS[hits_col]:-}" ]; then + pc_tbl=hits_col_pcopy + $PSQL -c "DROP TABLE IF EXISTS $pc_tbl;" >/dev/null 2>&1 + ddl_for "$pc_tbl" "USING pgcolumnar" | $PSQL -v ON_ERROR_STOP=1 >/dev/null 2>"$CB_DATA/ddl.pcopy.err" + if [ -s "$CB_DATA/ddl.pcopy.err" ]; then + head -5 "$CB_DATA/ddl.pcopy.err"; die "parallel arm DDL failed" + fi + t0=$(date +%s.%N) + $PSQL -v ON_ERROR_STOP=1 \ + -c "SELECT pgcolumnar.parallel_copy('$pc_tbl'::regclass, '$TSV', $CB_PCOPY_WORKERS)" \ + > "$CB_DATA/load.pcopy.log" 2>&1 \ + || { tail -10 "$CB_DATA/load.pcopy.log"; die "the parallel arm failed; it is not reported as a fast load"; } + $PSQL -c "VACUUM ANALYZE $pc_tbl;" >/dev/null 2>&1 + t1=$(date +%s.%N) + LOAD_S[columnar_pcopy]=$(awk -v a="$t0" -v b="$t1" 'BEGIN { printf "%.1f", b - a }') + ROWS[columnar_pcopy]=$($PSQL -At -c "SELECT count(*) FROM $pc_tbl") + SIZE_B[columnar_pcopy]=$($PSQL -At -c "SELECT pg_total_relation_size('$pc_tbl')") + note " columnar_pcopy (${CB_PCOPY_WORKERS}w): ${LOAD_S[columnar_pcopy]}s, ${ROWS[columnar_pcopy]} rows, ${SIZE_B[columnar_pcopy]} bytes" + + # The assertion that stops a failed parallel load being published as a fast + # one. cb_rows_ok refuses a missing count rather than comparing it, so a psql + # that died does not compare "" with "" and pass (#418). + if cb_rows_ok "${ROWS[columnar_pcopy]}" "$TSV_ROWS"; then + printf 'ok premise: %s\n' "the parallel arm loaded every row of the file" + else + printf 'FAIL premise: %s (got [%s] want [%s])\n' \ + "the parallel arm loaded every row of the file" \ + "${ROWS[columnar_pcopy]}" "$TSV_ROWS" >&2 + fail=1 + fi +fi + # Every arm must hold the same rows as the file. A load that silently dropped # rows makes every query below faster and wrong. for arm in "${ARMS[@]}"; do @@ -616,6 +693,12 @@ printf '%-16s %12s %16s %10s\n' arm 'load (s)' 'size (bytes)' 'errors' for arm in "${ARMS[@]}"; do printf '%-16s %12s %16s %10s\n' "$arm" "${LOAD_S[$arm]}" "${SIZE_B[$arm]}" "${ERRS[$arm]:-0}" done +if [ -n "${LOAD_S[columnar_pcopy]:-}" ]; then + printf '%-16s %12s %16s %10s\n' "columnar_pcopy" \ + "${LOAD_S[columnar_pcopy]}" "${SIZE_B[columnar_pcopy]}" "-" + printf ' parallel_copy with %s workers, beside the serial number above; both are the point\n' \ + "$CB_PCOPY_WORKERS" +fi echo echo "-- hot times, milliseconds. 'x' is columnar over heap; above 1.00 means we lose." printf '%-6s' query diff --git a/test/bench_guards.sh b/test/bench_guards.sh new file mode 100755 index 0000000..81a7f90 --- /dev/null +++ b/test/bench_guards.sh @@ -0,0 +1,112 @@ +#!/usr/bin/env bash +# +# The benchmark's own guards (#465). +# +# bench/run_clickbench.sh publishes numbers. A benchmark that reports a failed +# load as a fast one is worse than a benchmark that does not run, because the +# number reaches documentation and nobody re-derives it. +# +# That is not hypothetical. pgcolumnar.parallel_copy prepares one transaction per +# worker, and the stock max_prepared_transactions is 0, so EVERY parallel arm +# errors out instantly. The first harness written against it printed those +# failures as +# +# 0.0s / 0.8s / 1.1s / 1.3s +# +# which is indistinguishable from perfect scaling, and is the shape a reader +# would publish. #465 records it. +# +# So the guards get a suite of their own, and it runs in the matrix even though +# the benchmark it guards does not: the benchmark needs a 15 GB download and a +# tuned cluster, while its arithmetic needs neither. Testing the decision without +# the dataset is the whole point of putting it in a file that can be sourced. +# +# Usage: test/bench_guards.sh [PG_CONFIG] +# The argument is accepted and ignored; this suite needs no cluster. +# Written fresh for pgColumnar. +set -uo pipefail +SRCDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" + +PGC_CHECKS=0 +PGC_FAIL=0 +check() { + local name="$1" got="$2" want="$3" + PGC_CHECKS=$((PGC_CHECKS + 1)) + if [ "$got" = "$want" ]; then + echo "PASS $name" + else + echo "FAIL $name: got [$got] want [$want]" + PGC_FAIL=1 + fi +} + +GUARDS="$SRCDIR/bench/cb_guards.sh" +if [ ! -f "$GUARDS" ]; then + echo "FAIL bench/cb_guards.sh is missing, so the benchmark's guards are untestable" + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + echo; echo "checks run: $PGC_CHECKS"; echo "$(basename "$0"): FAILED"; exit 1 +fi +# shellcheck source=/dev/null +. "$GUARDS" + +echo "== pgColumnar test: $(basename "$0") ==" + +# ---- max_prepared_transactions must be preflighted, not discovered ---------- +# +# Raising it needs a postmaster restart, so finding out during the load means the +# whole run is wasted. The guard has to answer before any arm is loaded. +check "the stock 0 is refused for an 8-worker parallel arm" \ + "$(cb_prepared_xacts_ok 0 8 && echo ok || echo refused)" "refused" +check "an exact match is accepted" \ + "$(cb_prepared_xacts_ok 8 8 && echo ok || echo refused)" "ok" +check "more than enough is accepted" \ + "$(cb_prepared_xacts_ok 16 8 && echo ok || echo refused)" "ok" +check "one short is refused, which is the off-by-one that would run 7 workers" \ + "$(cb_prepared_xacts_ok 7 8 && echo ok || echo refused)" "refused" + +# A serial arm needs none, and must not be blocked by a setting it does not use. +check "a serial arm needs no prepared transactions" \ + "$(cb_prepared_xacts_ok 0 0 && echo ok || echo refused)" "ok" + +# The message is the deliverable here: the operator has to know WHAT to set and +# that it costs a restart. A bare "failed" sends them to the load log, which +# reports a per-worker error and not the cause. +msg="$(cb_prepared_xacts_message 0 8)" +check "the message names the setting to change" \ + "$([ "$(grep -c 'max_prepared_transactions' <<<"$msg")" -ge 1 ] && echo yes || echo no)" "yes" +check "and the number it must reach" \ + "$([ "$(grep -c '8' <<<"$msg")" -ge 1 ] && echo yes || echo no)" "yes" +check "and says it needs a restart, which is why this runs first" \ + "$([ "$(grep -ci 'restart' <<<"$msg")" -ge 1 ] && echo yes || echo no)" "yes" + +# ---- a load that lost rows is a failure, not a fast result ------------------ +check "a short load is refused" \ + "$(cb_rows_ok 1999999 2000000 && echo ok || echo refused)" "refused" +check "an empty load is refused, which is what an errored parallel arm produces" \ + "$(cb_rows_ok 0 2000000 && echo ok || echo refused)" "refused" +check "an exact load is accepted" \ + "$(cb_rows_ok 2000000 2000000 && echo ok || echo refused)" "ok" +# Empty is not zero. A psql that failed produces neither. +# +# BOTH sides empty is the case that matters, and it is the only one of the three +# that a plain `[ "$got" = "$want" ]` gets wrong: one empty side is unequal to a +# number and is refused either way. The first version of this suite asserted only +# the one-sided cases, and a removal proof showed they passed with the numeric +# check deleted, which means they were testing nothing. That is the trap #418 +# exists to forbid, met while writing the test for it. +check "two missing measurements are refused, not called equal" \ + "$(cb_rows_ok '' '' && echo ok || echo refused)" "refused" +check "a missing count is refused rather than compared with the expectation" \ + "$(cb_rows_ok '' 2000000 && echo ok || echo refused)" "refused" +check "and a missing expectation is refused too" \ + "$(cb_rows_ok 2000000 '' && echo ok || echo refused)" "refused" + +echo +echo "checks run: $PGC_CHECKS" +if [ "$PGC_FAIL" != 0 ]; then + echo "$(basename "$0"): FAILED" + exit 1 +fi +echo "$(basename "$0"): PASSED" +exit 0 diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 0b3e4f6..9315b49 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -48,6 +48,7 @@ SUITES=( arrow_nested arrow_nested_import audit + bench_guards bloom_lazy bloom_setting bloom_sizing