From fef4a70a1ad563b51205e5b91ad8a00fb67b205d Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 6 Aug 2026 20:48:39 -0600 Subject: [PATCH] bench: load a parallel arm beside the serial one, and guard it (#465) bench/ loaded every engine with a single serial COPY, so our own benchmark understated our own loader by seven times: 87.2s serial against 12.4s at 16 workers on 2M ClickBench rows, measured on #465, with identical stored size. The arm is reported BESIDE the serial number rather than replacing it. Serial COPY is the single-connection ingest path and parallel_copy is the bulk path; both are real capabilities and both are worth publishing. It is deliberately not in ARMS, because the 43 queries have nothing to say about a table that exists to be loaded, and running them would double the run for no reading. The guarding is the part that mattered. parallel_copy prepares one transaction per worker and the stock max_prepared_transactions is 0, so every parallel arm errors on its first worker and returns in about no time. #465 records a harness that printed those failures as 0.0s / 0.8s / 1.1s / 1.3s which is indistinguishable from perfect scaling and is exactly the shape someone would publish. Three things now stop it: the cluster is configured with one slot per worker, a preflight refuses to start when the RUNNING cluster cannot supply them (asked of the cluster, not of the file this script wrote, because an operator may point it at their own), and the loaded row count is asserted exactly. The guards live in bench/cb_guards.sh so the matrix can test them. The benchmark needs a 15 GB download and a tuned cluster; its arithmetic needs neither, and a decision that cannot be tested without the dataset would never be tested. test/bench_guards.sh covers them in the ordinary five-major run. Both guards are verified by removal. Weakening the preflight to always-true fails two checks; weakening the row check to a plain string compare fails one. That second proof caught a hole in my own test. The first version asserted only that ONE missing side is refused, and both of those cases pass with the numeric check deleted, because an empty string is unequal to a number either way. The case that distinguishes them is BOTH sides missing, which a plain compare calls equal -- the #418 trap, met while writing the test for it. The suite now asserts that one. Not covered here: the wiring itself is not exercised end to end, because this machine has no ClickBench dataset. What is proven is the arithmetic of the guards, that the source line resolves from another working directory, and that the script parses. The 7x figure is #465's measurement, not a new one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky --- bench/cb_guards.sh | 58 ++++++++++++++++++++ bench/run_clickbench.sh | 83 +++++++++++++++++++++++++++++ test/bench_guards.sh | 112 +++++++++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 1 + 4 files changed, 254 insertions(+) create mode 100644 bench/cb_guards.sh create mode 100755 test/bench_guards.sh 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