ci(tap): enable ClickHouse tests on TAP suite by JoshDreamland · Pull Request #101 · ClickHouse/pg_stat_ch

JoshDreamland · 2026-06-01T14:58:20Z

TL;DR

Some TAP tests apparently never ran on CI before. Now they do.

Drop --name clickhouse-test; use psch-clickhouse from compose file

Summary

CI was starting ClickHouse with docker run --network host on default ports 8123 / 9000, but every TAP test probes localhost:18123 and connects on localhost:19000 — the host-mapped ports from docker/docker-compose.test.yml.
The probe at the top of each ClickHouse-dependent test (plan skip_all => 'ClickHouse container not running' when localhost:18123 is unreachable) therefore failed in CI, and prove happily reported "All tests successful" with the integration coverage silently disabled.
This switches the start/cleanup steps to docker compose -f docker/docker-compose.test.yml up/down so CI gets the same port mapping, container name (psch-clickhouse), and healthcheck the suite already expects, and aligns CI with local dev.

Tests that were silently skipped on CI before this

From the most recent green TAP job on main / PR 92:
010_clickhouse_export, 011_clickhouse_reconnect, 018_producer_consumer, 021_cmd_type_counts, 023_drain_loop, 024_otel_export (separate — OTel collector is its own gap), 025_otel_reconnect, 027_query_normalization, 031_normalize_cache, single-cycle path in 016_boundary_conditions.

Note

Medium Risk
CI behavior change exposes previously skipped integration tests; macOS link semantics affect runtime symbol binding for the extension.

Overview
CI TAP now starts ClickHouse with docker compose -f docker/docker-compose.test.yml (host ports 18123 / 19000, container psch-clickhouse) instead of a host-network docker run on 8123, so the suite’s readiness checks and docker exec match what the Perl tests already use. Failure artifacts upload tmp_check/ and log/; cleanup uses compose down -v.

Build treats pg_stat_ch as a PostgreSQL module (add_library(... MODULE)), and on macOS links with -bundle_loader to the postgres binary from pg_config instead of -undefined dynamic_lookup, so PG symbols resolve against the server executable and avoid clashes with system libraries.

^{Reviewed by Cursor Bugbot for commit 42c972d. Bugbot is set up for automated code reviews on this repo. Configure here.}

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

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

+  target_link_options(pg_stat_ch PRIVATE
+    -bundle_loader ${PG_BINDIR}/postgres
+    -Wl,-undefined,dynamic_lookup)


Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit eaa9073. Configure here.}

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

…TAP suite The TAP harness probes localhost:18123 and connects on localhost:19000 — the host ports defined in docker/docker-compose.test.yml. CI was launching ClickHouse with `docker run --network host` on the default 8123/9000, so every ClickHouse-dependent test (010, 011, 018, 021, 023, 027, 031, single-cycle 016) hit `plan skip_all => 'ClickHouse container not running'` and prove reported "All tests successful" with the integration coverage silently disabled. Switching the start/cleanup steps to `docker compose ... up/down` against the existing docker-compose.test.yml gets the ports, container name, and healthcheck the suite already expects, and aligns CI with local dev. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ch to libSystem On macOS, our extension was linked with -undefined dynamic_lookup only, which lets dyld resolve any undefined symbol against whichever loaded dylib exports the name. libSystem.B.tbd exports _hash_search, _hash_create, and _hash_destroy (its POSIX hsearch family) - and dyld was binding our calls to those instead of PostgreSQL's own dynahash. The signatures don't match at all, so PschPostParseAnalyze -> PschRememberNormalizedQuery -> hash_search corrupted libSystem's hash table internals and SIGTRAP'd inside libsystem_malloc on the next allocation. Every macOS-built copy of this extension had the bug; CI is Linux and glibc doesn't export hash_search, so it never surfaced there. PGXS's own Makefile.port handles this by passing -bundle_loader $(bindir)/postgres on Darwin, which resolves undefined symbols against the actual postgres binary rather than libSystem. -bundle_loader is only valid with -bundle, which CMake produces with add_library(... MODULE) rather than SHARED. We also keep -undefined dynamic_lookup so transitive dylib deps (gRPC inside opentelemetry-cpp, etc.) can stay runtime-resolved as before. Verified locally: t/010, t/011, t/021, t/027, t/031 - all previously silently-skipped ClickHouse-dependent TAP tests - now pass against PG 18.3 + ClickHouse 26.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ropped silently

…ght fix Per Copilot review: -Wl,-undefined,dynamic_lookup switches the binary back to flat-namespace runtime lookup, which is exactly the mechanism that let libSystem's hash_search bind ahead of PostgreSQL's. -bundle_loader already establishes two-level namespace resolution against the postgres executable; dynamic_lookup undoes that. PGXS's own Makefile.port doesn't combine the two, and any symbol genuinely missing from postgres should fail at link time rather than silently bind to an unrelated dylib at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…obody would make

PostgreSQL::Test::Cluster's _logfile is ${log_path}/<name>.log where log_path = $ENV{TESTLOGDIR} // 'log' (relative to prove's CWD, i.e. the project root). Last run uploaded tmp_check/ but the pgdata subdirs there don't contain the bgworker WARNING messages we need to diagnose the export failures.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

…yhash overlay), #101 (CI ClickHouse-test enablement) so PR #92 builds + tests against the corrected dep set

Copilot AI review requested due to automatic review settings June 1, 2026 14:58

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 14:58 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

JoshDreamland changed the title ~~ci(tap): start ClickHouse via docker-compose so ports match the TAP suite~~ ci(tap): fix and enable ClickHouse tests on TAP suite Jun 1, 2026

Copilot stopped reviewing on behalf of JoshDreamland due to an error June 1, 2026 15:09
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

theory approved these changes Jun 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 1, 2026 16:51

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 16:51 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Comment thread CMakeLists.txt Outdated

Comment on lines +126 to +128

target_link_options(pg_stat_ch PRIVATE

-bundle_loader ${PG_BINDIR}/postgres

-Wl,-undefined,dynamic_lookup)

Copilot AI review requested due to automatic review settings June 1, 2026 17:04

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 17:04 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 1, 2026 18:14

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 18:14 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

cursor Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread docker/postgres-ext.Dockerfile Outdated

Copilot AI review requested due to automatic review settings June 1, 2026 19:16

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 19:16 View session

JoshDreamland mentioned this pull request Jun 1, 2026

fix(ci): pin vcpkg checkout to tag 2026.04.27 so the binary cache actually hits #102

Merged

1 task

Copilot AI reviewed Jun 1, 2026

View reviewed changes

serprex approved these changes Jun 1, 2026

View reviewed changes

JoshDreamland force-pushed the ci-tap-clickhouse-ports branch from d76e454 to 4902ec6 Compare June 1, 2026 19:58

JoshDreamland mentioned this pull request Jun 1, 2026

fix(vcpkg): update overlay to use correct cityhash on the wire #103

Merged

2 tasks

JoshDreamland and others added 6 commits June 1, 2026 17:29

ci(tap): upload tmp_check/ from project root so failure logs aren't d…

d6c93e6

…ropped silently

ci(tap): drop the tmp_check path comment; it argued against a guess n…

d9f0680

…obody would make

Copilot AI review requested due to automatic review settings June 1, 2026 21:29

JoshDreamland force-pushed the ci-tap-clickhouse-ports branch from 4902ec6 to 42c972d Compare June 1, 2026 21:29

Copilot started reviewing on behalf of JoshDreamland June 1, 2026 21:30 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

JoshDreamland changed the title ~~ci(tap): fix and enable ClickHouse tests on TAP suite~~ ci(tap): enable ClickHouse tests on TAP suite Jun 1, 2026

JoshDreamland merged commit 274bfca into main Jun 1, 2026
13 checks passed

JoshDreamland deleted the ci-tap-clickhouse-ports branch June 1, 2026 21:47

JoshDreamland added a commit that referenced this pull request Jun 1, 2026

Merge main into dsa-query-intern: pick up #102 (vcpkg pin), #103 (cit…

2332b9a

…yhash overlay), #101 (CI ClickHouse-test enablement) so PR #92 builds + tests against the corrected dep set

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(tap): enable ClickHouse tests on TAP suite#101

ci(tap): enable ClickHouse tests on TAP suite#101
JoshDreamland merged 6 commits into
mainfrom
ci-tap-clickhouse-ports

JoshDreamland commented Jun 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

JoshDreamland commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Summary

Tests that were silently skipped on CI before this

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoshDreamland commented Jun 1, 2026 •

edited

Loading