Skip to content

chore: reduce LOC by ~20% — drop dead benchmark/perf/unit-test infra#80

Merged
iskakaushik merged 1 commit intomainfrom
tech-debt
Apr 17, 2026
Merged

chore: reduce LOC by ~20% — drop dead benchmark/perf/unit-test infra#80
iskakaushik merged 1 commit intomainfrom
tech-debt

Conversation

@iskakaushik
Copy link
Copy Markdown
Collaborator

Summary

  • −20.6% code LOC (13,556 → 10,766, net −3,674 file bytes across 35 files)
  • Removes entirely dead/duplicative infra; no functional changes to the extension

What's deleted

Benchmark + perf infra (loadgen package, perf Docker rig, entrypoints):

  • benchmark/ (uv-managed Python loadgen pkg, docker-compose, init SQL)
  • scripts/bench-otel.sh, scripts/run-perf.sh
  • docker/perf.Dockerfile, docker/perf-entrypoint.sh, docker/docker-compose.perf.yml
  • CI job bench-otel, mise task perf

C++ unit tests (gtest harness):

  • test/unit/arrow_batch_test.cc (522 LOC) + test/unit/hostname_test.cc (65 LOC)
  • gtest removed from vcpkg.json
  • CI job unit-tests, mise task test:unit

Overlapping TAP tests:

  • 028/029/030 query_normalization variants folded into 027 as subtests (preserves all unique assertions: error-path leak, escape_string_warning dedupe, nested SPI normalization)
  • Dropped 003_buffer_overflow (covered by overflow_race.spec + 017_toctou_race.pl)
  • Dropped 013_buffer_metrics (ClickHouse-gated tautologies; buffer capture is already in buffers.sql)
  • Dropped 014_cpu_metrics (no unique signal beyond >= 0 checks)
  • Dropped 019_reset_atomicity (reset during concurrent writes covered by overflow_race + ring_buffer_concurrent specs)

Kept (unique coverage the specs can't reproduce):

  • 017_toctou_race.pl — double-checked-locking race under real pgbench contention
  • 022_overflow_deadlock.plemit_log_hookPschEnqueueEvent re-entry deadlock

Test plan

  • cmake --build build clean against PG 18.3 local install (TAP-enabled)
  • Regression suite (10/10 pass): basic, version, guc, stats, utility, buffers, cmd_type, client_info, error_capture, drop_database_barrier
  • Isolation suite (3/3 pass): ring_buffer_concurrent, ring_buffer_boundary, overflow_race
  • CI green (will verify after push)

Not included

overlay-ports/cityhash/ — discussed but deferred. The overlay patches a real aarch64 config.guess bug upstream hasn't fixed, and clickhouse-cpp pulls cityhash transitively via vcpkg. Full submodule vendoring would need an additional clickhouse-cpp port patch to drop the transitive dep, which adds LOC rather than removing it. Happy to do this as a follow-up if you want a specific shape.

Delete code that's either unreferenced, replaced by better coverage, or
was effectively dead:

- benchmark/ (loadgen python package, uv.lock, docker-compose, init sql)
- Perf-profiling infra: scripts/{bench-otel,run-perf}.sh,
  docker/perf.Dockerfile, docker/perf-entrypoint.sh,
  docker/docker-compose.perf.yml (all tied to benchmark/)
- test/unit/ gtest harness (arrow_batch + hostname) + gtest from vcpkg.json
- CI jobs: unit-tests, bench-otel
- mise tasks: test:unit, perf

Consolidate overlapping TAP tests:

- Fold 028/029/030 query_normalization variants into 027 as subtests
- Drop 003_buffer_overflow (covered by overflow_race spec + 017 TOCTOU)
- Drop 013_buffer_metrics (ClickHouse-gated tautologies; buffer capture
  already covered by test/regression/sql/buffers.sql)
- Drop 014_cpu_metrics (no unique signal beyond >= 0 assertions)
- Drop 019_reset_atomicity (reset-during-write covered by overflow_race
  and ring_buffer_concurrent specs)

Kept: 017_toctou_race (unique double-checked-locking race coverage) and
022_overflow_deadlock (unique emit_log_hook re-entry coverage).

Net: 13,556 -> 10,766 code LOC (-20.6%, -3,674 total file bytes).

Regression (10/10) + isolation (3/3) remain green against PG 18.3.
Copilot AI review requested due to automatic review settings April 17, 2026 01:01
@iskakaushik iskakaushik merged commit 317bdfa into main Apr 17, 2026
14 checks passed
@iskakaushik iskakaushik deleted the tech-debt branch April 17, 2026 01:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes dead/duplicative benchmark, perf, and unit-test infrastructure to substantially reduce LOC while keeping the extension’s runtime behavior unchanged.

Changes:

  • Drops perf/benchmark Docker + scripts and the Python benchmark/loadgen package.
  • Removes opt-in C++ gtest unit tests and related build/CI/task wiring (including gtest vcpkg dep).
  • Consolidates overlapping TAP query-normalization tests into t/027_query_normalization.pl and deletes redundant TAP files.

Reviewed changes

Copilot reviewed 33 out of 35 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vcpkg.json Removes gtest dependency now that C++ unit tests are dropped.
test/unit/hostname_test.cc Deletes gtest-based hostname unit test.
test/unit/arrow_batch_test.cc Deletes gtest-based Arrow batch unit test.
t/030_nested_query_normalization.pl Removes standalone nested normalization TAP test (folded into t/027).
t/029_query_normalization_warning.pl Removes standalone warning-dedup TAP test (folded into t/027).
t/028_query_normalization_error_leak.pl Removes standalone error-path leak TAP test (folded into t/027).
t/027_query_normalization.pl Adds new subtests to absorb coverage from removed 028/029/030 variants.
t/019_reset_atomicity.pl Deletes TAP reset atomicity test deemed redundant with isolation specs.
t/014_cpu_metrics.pl Deletes CPU metrics TAP test.
t/013_buffer_metrics.pl Deletes buffer/WAL metrics TAP test.
t/003_buffer_overflow.pl Deletes overflow TAP test deemed redundant with isolation/spec coverage.
scripts/run-perf.sh Removes perf benchmark runner script.
scripts/bench-otel.sh Removes OTel throughput benchmark script.
mise.toml Removes test:unit and perf tasks.
docs/guides/testing.md Updates testing guide to remove deleted TAP tests from the list.
docker/perf.Dockerfile Removes perf benchmark Docker image build.
docker/perf-entrypoint.sh Removes perf benchmark container entrypoint.
docker/docker-compose.perf.yml Removes perf docker-compose stack.
benchmark/loadgen/uv.lock Deletes lockfile for removed Python load generator.
benchmark/loadgen/src/loadgen/workloads.py Deletes workload definitions for removed load generator.
benchmark/loadgen/src/loadgen/runner.py Deletes runner for removed load generator.
benchmark/loadgen/src/loadgen/cli.py Deletes CLI for removed load generator.
benchmark/loadgen/src/loadgen/__init__.py Deletes package init for removed load generator.
benchmark/loadgen/pyproject.toml Deletes Python project definition for removed load generator.
benchmark/loadgen/README.md Deletes documentation for removed load generator.
benchmark/loadgen/.python-version Deletes Python version pin for removed load generator.
benchmark/loadgen/.gitignore Deletes subproject ignore rules for removed load generator.
benchmark/init/01-create-extension.sql Deletes benchmark DB init SQL.
benchmark/docker-compose.yml Deletes benchmark docker-compose stack.
benchmark/README.md Deletes benchmark environment documentation.
benchmark/.env.example Deletes benchmark env template.
META.json Removes benchmark from no_index directories after benchmark removal.
CMakeLists.txt Removes PSCH_BUILD_UNIT_TESTS option and unit test targets.
.gitignore Drops ignores related to removed unit/perf/benchmark infra.
.github/workflows/ci.yml Removes CI jobs for unit tests and perf benchmark.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +294 to +300
my $error_query = psch_query_clickhouse(
"SELECT query FROM pg_stat_ch.events_raw " .
"WHERE err_message != '' " .
"ORDER BY ts_start DESC LIMIT 1"
);
is($error_query, '', 'Error events do not export query text');

Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new error-path leak subtest, the assertion that the error event exports an empty query can pass even if the error event was never exported/inserted into ClickHouse (the SELECT may return no rows, yielding an empty string). Add an explicit wait/assert that at least one error row exists (e.g., poll count() on err_message != '') and/or wait for a minimum exported count before checking query == '' so the test actually validates the behavior and isn’t timing-dependent.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants