Skip to content

Replace clickhouse-cpp with clickhouse-c#104

Open
serprex wants to merge 1 commit into
mainfrom
clickhouse-c
Open

Replace clickhouse-cpp with clickhouse-c#104
serprex wants to merge 1 commit into
mainfrom
clickhouse-c

Conversation

@serprex
Copy link
Copy Markdown
Member

@serprex serprex commented Jun 2, 2026

Wraps clickhouse-c in C++ exporter interface,
relying on PG MemoryContext, but with NO_OOM avoiding longjmp in C++ contexts

Avoid more allocations by not recreating columns on each batch


Note

High Risk
Large rewrite of the bgworker ClickHouse export path (sockets, TLS, protocol, memory) plus dependency and CI changes; regressions could drop or corrupt telemetry or stall the worker on I/O.

Overview
Replaces clickhouse-cpp (vcpkg overlay port) with the vendored clickhouse-c submodule: a single CHC_IMPLEMENTATION TU, CMake links lz4/zstd/OpenSSL directly, and the ClickHouse exporter is rewritten to use native protocol I/O (POSIX or OpenSSL), LZ4 compression, chc_block_builder INSERTs, and PostgreSQL MemoryContext allocators with MCXT_ALLOC_NO_OOM so OOM does not longjmp through C++.

Batching behavior changes: column objects are created once and Clear() on each batch instead of rebuilding clickhouse-cpp blocks/columns every flush. Connection setup adds bounded TCP connect, read deadlines, and TLS (SNI, verify or skip per GUC) with cancel checks tied to ProcDiePending.

CI and local tests now init the clickhouse-c submodule, bump several GitHub Actions, drop overlay-port cache keys, and start ClickHouse with generated self-signed TLS (docker-compose.tls.yml, generate-certs.sh). New TAP t/013_clickhouse_tls.pl and psch_init_node_with_clickhouse TLS GUCs exercise encrypted export and reconnect; ClickHouse integration runs match t/*clickhouse*.pl.

Docs, Makefile, and Docker builds are updated for the new dependency layout; FindClickHouseCpp and the clickhouse-cpp overlay are removed.

Reviewed by Cursor Bugbot for commit 2b5fdb7. Bugbot is set up for automated code reviews on this repo. Configure here.

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

This PR migrates the ClickHouse export backend from the clickhouse-cpp client to the vendored, header-only clickhouse-c library, integrating it behind the existing C++ StatsExporter interface while using PostgreSQL MemoryContext allocation (with MCXT_ALLOC_NO_OOM) to avoid longjmp across C++ frames and to reuse column buffers between batches.

Changes:

  • Replaced the ClickHouse exporter implementation to use clickhouse-c (native protocol, custom TCP/TLS + bounded I/O) and added a single CHC_IMPLEMENTATION translation unit.
  • Updated build/dependency plumbing: removed clickhouse-cpp vcpkg overlay port and added explicit lz4/openssl dependencies and linking.
  • Updated docs, CI, and quickstart scripts to initialize the new third_party/clickhouse-c submodule.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
vcpkg.json Drops clickhouse-cpp dependency; adds lz4/openssl alongside zstd.
vcpkg-configuration.json Removes overlay-ports usage (keeps overlay triplets).
src/export/stats_exporter.cc Removes clickhouse-cpp include; updates shutdown comment to reflect remaining destructor risk.
src/export/exporter_interface.h Adds BasicColumn::Clear() hook to enable per-batch buffer reuse.
src/export/clickhouse_exporter.cc Reimplements ClickHouse exporter using clickhouse-c, PG allocators, custom TCP/TLS connect, and binary INSERT flow.
src/export/clickhouse_c_impl.c Adds the single CHC_IMPLEMENTATION compilation unit for clickhouse-c.
scripts/quickstart.sh Ensures clickhouse-c submodule is initialized before building the Docker stack.
README.md Updates dependency note from clickhouse-cpp to clickhouse-c.
Makefile Updates build flow to ensure clickhouse-c submodule is present and uses CMake build/install.
INSTALL.md Updates documentation to reference clickhouse-c as statically linked dependency.
docker/postgres-ext.Dockerfile Copies vendored third_party/clickhouse-c into the build context; removes overlay-ports copy.
CMakeLists.txt Removes ClickHouseCpp find/link; adds lz4/zstd find + links; adds SYSTEM include for vendored clickhouse-c.
cmake/FindClickHouseCpp.cmake Removed (no longer needed).
cmake/CompilerWarnings.cmake Updates comment to reflect exceptions/RTTI rationale (OTel/Arrow instead of clickhouse-cpp).
CLAUDE.md Updates dependency documentation and records clickhouse-c embedding pattern and implementation TU location.
.gitmodules Adds third_party/clickhouse-c submodule entry.
.github/workflows/release.yml Initializes clickhouse-c submodule in release workflow.
.github/workflows/ci.yml Initializes clickhouse-c submodule in CI jobs.
.github/workflows/ci-tap.yml Initializes clickhouse-c submodule in TAP workflow.
.github/actions/setup-vcpkg/action.yml Updates vcpkg cache key to stop hashing removed overlay-ports/**.
overlay-ports/clickhouse-cpp/vcpkg.json Removed obsolete vcpkg overlay port definition.
overlay-ports/clickhouse-cpp/portfile.cmake Removed obsolete vcpkg overlay port build script.
overlay-ports/clickhouse-cpp/fix-deps-and-build-type.patch Removed obsolete overlay patch.

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

Comment thread src/export/clickhouse_exporter.cc Outdated
Comment thread src/export/clickhouse_exporter.cc Outdated
Comment thread src/export/clickhouse_exporter.cc
Comment thread src/export/clickhouse_exporter.cc
Comment thread src/export/clickhouse_exporter.cc
Copilot AI review requested due to automatic review settings June 2, 2026 07:11
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

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

Comment thread src/export/clickhouse_exporter.cc
Copy link
Copy Markdown
Contributor

@theory theory left a comment

Choose a reason for hiding this comment

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

Yay!

Comment thread .github/workflows/ci.yml
Comment thread scripts/run-tests.sh Outdated
Comment thread INSTALL.md
Comment thread Makefile
Comment thread Makefile
Comment thread src/export/clickhouse_exporter.cc
Copilot AI review requested due to automatic review settings June 2, 2026 14:40
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

Copilot reviewed 31 out of 33 changed files in this pull request and generated 4 comments.

Comment thread src/export/clickhouse_exporter.cc
Comment thread src/export/clickhouse_exporter.cc
Comment thread src/export/clickhouse_exporter.cc Outdated
Comment thread t/psch.pm
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

Copilot reviewed 31 out of 33 changed files in this pull request and generated 2 comments.

Comment thread src/export/clickhouse_exporter.cc
Comment thread src/export/clickhouse_exporter.cc Outdated
Copilot AI review requested due to automatic review settings June 3, 2026 00:17
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

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

Fix All in Cursor

❌ 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 dab1351. Configure here.

Comment thread src/export/clickhouse_exporter.cc
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

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

Comments suppressed due to low confidence (1)

docker/docker-compose.tls.yml:10

  • This file overrides the ClickHouse service healthcheck from docker-compose.test.yml, but it omits interval, timeout, and retries. Docker Compose will fall back to much slower defaults (e.g. 30s interval), which can make docker compose up --wait noticeably slower or flaky in CI/local test runs.

Consider copying the base healthcheck timing parameters here so enabling TLS doesn’t change startup/health semantics.

    healthcheck:
      test: ["CMD", "clickhouse-client", "--secure", "--host", "localhost", "--port", "9440", "--accept-invalid-certificate", "-q", "SELECT 1"]

Wraps clickhouse-c in C++ exporter interface, relying on PG MemoryContext,
but with NO_OOM avoiding longjmp in C++ contexts

Avoid more allocations by not recreating columns on each batch
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