fix: make the benchmarks measure what they claim, before going public - #44
Merged
Conversation
Three of the four benchmarks were reporting numbers that did not support their own thesis. Verified against a real PostgreSQL 17 instance. - pagination held OFFSET at 500000 and varied the page size instead, so on the small and medium datasets both queries returned zero rows while the chart still rendered. It now varies the offset with a fixed page size. - index_usage captured both EXPLAIN plans after CREATE INDEX, making them byte-identical and the "no index" label false. The unindexed plan is now captured before the index exists. - select_star swept LIMITs from 1M down regardless of table size, so every data point returned the whole table. Limits now derive from the real row count, and rows_fetched is shown in the comparison table. - join_vs_subquery dropped and recreated a table called `orders`, which would destroy a user table of that name. Renamed to sqlperf_orders. All four now share measure(), which discards a warm-up run and reports the median of five. Previously each point was a single cold run with the slow query always going first, biasing every result toward its own conclusion. required_tables is now enforced by check_requirements() instead of being a decorative attribute, and BenchmarkNotApplicable replaces silently charting an empty result set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo
The size parameter was validated, stored in metadata and rendered in the UI, but never reached the seeder: init.sql hardcoded 1M rows in a PL/pgSQL loop, sql/seed.py was never invoked by anything, and DB_SEED_SIZE was read into Config and unused. Every stored result recorded a size that had never been applied. - init.sql is now schema only; seeding belongs to sql/seed.py. - seed() is set-based (INSERT ... SELECT generate_series) instead of a row-by-row loop, which takes 1M rows from minutes to seconds and makes reseeding viable from a web request. - seed() is idempotent on the exact row count, so switching size reseeds in both directions. - A lock_timeout guards the TRUNCATE: a concurrent reader would otherwise block it forever, and a client-side timeout cannot interrupt libpq waiting on the socket, so the worker would hang. Also fixes an off-by-one in the old init.sql array indexing: casting a float to INT in PostgreSQL rounds rather than truncates, so `(random() * 10)::INT + 1` produced index 11 on a 10-element array. Measured over 100k samples, 4.9% of rows got NULL for name, surname, city and country. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo
- load_result() built a filesystem path straight from the URL segment. `GET /results/..` read a metadata.json outside results/ and returned its contents. Result ids are now validated against the results directory. - save_result() used a second-precision timestamp with exist_ok=True, so two runs in the same second silently overwrote each other. - routes called get_db_connection() with no arguments, so the app's configured database was ignored and Config() was rebuilt from the environment. The app config is now threaded through. - test_generate_route_without_db asserted `has_error or has_results`, where has_results checked for "Generate" — the button label, present on every page including the error path. The assertion could never fail. It now asserts the generic error is shown and that no internals leak. Drops the unused SECRET_KEY (no sessions or flash messages) and its hardcoded default, plus the dead TMP_DIR, START and STEP constants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo
.github/ existed but was empty: nothing verified the test suite on a PR, and the "tests passing" badge was a hand-written image. CI now runs ruff and pytest against a real PostgreSQL service, so the integration tests that guard the measurement contract cannot silently skip. - Coverage now includes benchmarks/ and sql/, which were excluded while the benchmark run() methods sat at 0%. Gate raised back from 50% to 70% (currently 93%). - docker-compose: dedicated seed service gated on db health, app gated on seed completion, no bind mount of the source tree (which put the real .env inside the container), app bound to 127.0.0.1, obsolete version key dropped. - .dockerignore no longer ships results/, .github/ or test caches. - Adds .python-version and requires-python; the badge said 3.11+, the Dockerfile 3.12 and the local venv 3.14. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo
The README documented features that did not exist and made claims the code did not support: - A dataset size table and DB_SEED_SIZE variable that were never wired up. - "SELECT * transfers 16x more data" — 16 columns versus 3 is not 16x of anything; the measured speedup is ~5x. - "Red line: without index (Seq Scan). Green line: with B-tree index (Index Scan)" while both stored plans were the same Seq Scan. - "OFFSET degrades linearly with page number" next to a chart whose x-axis was page size. - Test count given as 33 in one section and 36 in another. - A plugin example passing rows_fetched as an int where the dataclass wants a list, filtering on cities absent from the seed data, and documenting required_tables as required when nothing read it. Adds a section describing the measurement method and its limits, and an explicit warning that the tool truncates users and creates and drops its own tables. Benchmark images regenerated from the corrected code against 100k rows. Adds CONTRIBUTING.md and SECURITY.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes everything found in the pre-publication audit (
~/Repos/analysis/projects/sql-performance/audit-20260906.md). Every finding was reproduced against a real PostgreSQL 17 instance, and every fix has a test that was watched failing first.Why this is one PR
It is larger than the usual 400-line budget because the findings are entangled: the pagination and
select_starbugs cannot be fixed without first wiring up the dataset size, and none of it can be verified without the integration suite. Split into 5 reviewable commits.Blocking issues fixed
OFFSET 500000fixed, page size varied. On small/medium both queries returned 0 rows and still drew a chart.EXPLAINruns happened afterCREATE INDEX— byte-identical plans,cost=4.67..127.18both. The "no index" label was false.Seq Scan (cost=0.00..352.00)vsBitmap Heap Scan (cost=5.55..225.84).init.sqlhardcoded 1M rows,sql/seed.pywas called by nothing,DB_SEED_SIZEwas unused.Also fixed
GET /results/..read ametadata.jsonoutsideresults/and returned it.DROP TABLE orders: would destroy a user table of that name. Nowsqlperf_orders.(random() * 10)::INT + 1yields 11 on a 10-element array because the cast rounds. Measured 4.9% of rows with NULL name/surname/city/country.assert has_error or has_resultswherehas_resultschecked for"Generate", the button label present on every page. It could never fail.get_db_connection()with no arguments, discarding the app's configuration.TRUNCATEwaits forever; alock_timeoutnow bounds it.SECRET_KEYand its hardcoded default,TMP_DIR,START,STEP.required_tablesis now enforced rather than decorative.Verification
run()methods at 0%).ruff check .clean.docker composestack exercised end to end: all four benchmarks return 200 with charts and no errors..github/was empty, so nothing verified any of this on a PR.Note
The
landingandhistoryscreenshots still show the previous UI (the size selector labels changed) and are worth retaking before publishing. The four benchmark images were regenerated from the corrected code.🤖 Generated with Claude Code
https://claude.ai/code/session_014abw4B6YUf54giaEyPQpbo