Refactor exporter files: reduce noise and improve structure by iskakaushik · Pull Request #54 · ClickHouse/pg_stat_ch

iskakaushik · 2026-04-05T21:14:13Z

Summary

stats_exporter.cc: Remove 17 DEBUG2/DEBUG3 elog calls (bringup scaffolding), add ClampFieldLen<T> template to replace 3 inline clamping blocks, remove redundant comments and event_idx counter
otel_exporter.cc: Split EstablishNewConnection into CreateResource(), InitMetricsPipeline(), InitLogPipeline() private helpers; trim verbose CommitBatch comment
event.h: Add PSCH_MAX_APP_NAME_LEN and PSCH_MAX_CLIENT_ADDR_LEN constants to replace magic numbers

Net -61 lines, no behavioral changes.

Test plan

Incremental build passes (ninja compiles both .cc files cleanly)
mise run test:regress — SQL regression tests
mise run test:tap — TAP tests against install_tap build

🤖 Generated with Claude Code

…Tel init stats_exporter.cc: - Remove 17 DEBUG2/DEBUG3 elog calls (bringup scaffolding) - Add ClampFieldLen<T> template to replace 3 inline clamping blocks - Remove redundant section-header comments and event_idx counter otel_exporter.cc: - Split EstablishNewConnection into CreateResource(), InitMetricsPipeline(), and InitLogPipeline() private helpers - Trim 13-line CommitBatch comment to 2-line summary event.h: - Add PSCH_MAX_APP_NAME_LEN and PSCH_MAX_CLIENT_ADDR_LEN constants Net -61 lines, no behavioral changes.

Copilot

Pull request overview

Refactors the exporter implementation to reduce debug-log noise, improve code structure, and replace magic numbers with named constants used when exporting queue events.

Changes:

Introduces a reusable ClampFieldLen<T> helper and uses it for query/error/app/client length clamping in stats_exporter.cc.
Splits OTel connection setup into CreateResource(), InitMetricsPipeline(), and InitLogPipeline() helpers in otel_exporter.cc.
Adds named constants for application name and client address max lengths in event.h.

Reviewed changes

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

File	Description
src/queue/event.h	Adds max-length constants intended to replace magic numbers for fixed-size string fields.
src/export/stats_exporter.cc	Removes noisy DEBUG2/DEBUG3 logging and refactors repeated length-clamping logic into a helper.
src/export/otel_exporter.cc	Refactors `EstablishNewConnection()` into smaller helpers and trims an overly long comment.

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

Copilot · 2026-04-05T21:18:13Z

src/queue/event.h

+// Maximum application name length (NAMEDATALEN - 1)
+#define PSCH_MAX_APP_NAME_LEN 63
+
+// Maximum client address length (INET6_ADDRSTRLEN - 1)
+#define PSCH_MAX_CLIENT_ADDR_LEN 45


These constants are described as derived from NAMEDATALEN/INET6_ADDRSTRLEN, but the values are hard-coded (63/45). This can silently drift if those upstream macros change (or if the struct field sizes change). Prefer defining them in terms of the actual macros (e.g., NAMEDATALEN - 1 and INET6_ADDRSTRLEN - 1) so the code and comment can’t diverge.

Copilot · 2026-04-05T21:18:13Z

src/export/stats_exporter.cc

+LenT ClampFieldLen(LenT len, LenT max, const char* field_name) {
+  if (len <= max)
+    return len;
+  elog(WARNING, "pg_stat_ch: invalid %s %u, clamping", field_name, static_cast<unsigned>(len));


ClampFieldLen() warning message is missing key context: it doesn’t report the maximum allowed value, and it no longer includes any event identifier (e.g., pid/query_id). That makes diagnosing corrupted/invalid events much harder. Consider logging both the observed len and the max (and optionally pid/query_id from the caller) in the WARNING.

Suggested change

LenT ClampFieldLen(LenT len, LenT max, const char* field_name) {

if (len <= max)

return len;

elog(WARNING, "pg_stat_ch: invalid %s %u, clamping", field_name, static_cast<unsigned>(len));

LenT ClampFieldLen(LenT len, LenT max, const char* field_name, const char* event_context = nullptr) {

if (len <= max)

return len;

if (event_context != nullptr && event_context[0] != '\0') {

elog(WARNING,

"pg_stat_ch: invalid %s len=%llu max=%llu, clamping (%s)",

field_name,

static_cast<unsigned long long>(len),

static_cast<unsigned long long>(max),

event_context);

} else {

elog(WARNING,

"pg_stat_ch: invalid %s len=%llu max=%llu, clamping",

field_name,

static_cast<unsigned long long>(len),

static_cast<unsigned long long>(max));

}

Copilot · 2026-04-05T21:18:13Z

src/export/stats_exporter.cc

    col_db->Append(std::string(ev.datname, ev.datname_len));
    col_username->Append(std::string(ev.username, ev.username_len));


datname_len and username_len are used directly to size std::string, but they are not validated against the fixed buffers (datname[64], username[64]). If an event ever contains an out-of-range length, this becomes an out-of-bounds read (and potentially data exfiltration/crash). Since you’re already clamping other variable-length fields, please clamp these too (e.g., to sizeof(ev.datname)-1 / sizeof(ev.username)-1) before constructing strings.

Suggested change

col_db->Append(std::string(ev.datname, ev.datname_len));

col_username->Append(std::string(ev.username, ev.username_len));

auto dlen =

ClampFieldLen(ev.datname_len, static_cast<uint8>(sizeof(ev.datname) - 1), "datname_len");

auto ulen = ClampFieldLen(ev.username_len, static_cast<uint8>(sizeof(ev.username) - 1),

"username_len");

col_db->Append(std::string(ev.datname, dlen));

col_username->Append(std::string(ev.username, ulen));

Copilot · 2026-04-05T21:18:13Z

src/export/otel_exporter.cc

+  logs_sdk::BatchLogRecordProcessorOptions batch_opts;
+  batch_opts.max_queue_size = psch_otel_log_queue_size;
+  batch_opts.max_export_batch_size =
+      std::min(static_cast<size_t>(psch_otel_log_batch_size), batch_size_by_bytes);
+  batch_opts.schedule_delay_millis = std::chrono::milliseconds(psch_otel_log_delay_ms);


InitLogPipeline() uses std::min but this file doesn’t include . It may compile today via transitive includes, but that’s not guaranteed; please add an explicit #include to make the dependency clear and robust.

Add LogExporterWarning() bridge in exporter_interface.h — otel_exporter.cc cannot include postgres.h (macro conflicts with libintl.h via gRPC headers), so this bridges to ereport(WARNING) defined in stats_exporter.cc.

Copilot

Pull request overview

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

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

src/queue/event.h

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

src/queue/event.h

Co-authored-by: Philip Dubé <159546+serprex@users.noreply.github.com>

Copilot

Pull request overview

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

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

src/export/otel_exporter.cc

src/export/stats_exporter.cc

- event.h: define INET6_ADDRSTRLEN (46) if not already provided, avoiding dependency on <arpa/inet.h> which is missing in some build environments - hostname_test.cc: stub LogExporterWarning to fix linker error

Copilot AI review requested due to automatic review settings April 5, 2026 21:14

Copilot started reviewing on behalf of iskakaushik April 5, 2026 21:14 View session

Copilot AI reviewed Apr 5, 2026

View reviewed changes

iskakaushik force-pushed the refactor/exporter-cleanup branch from e5b2015 to aadbe0b Compare April 5, 2026 21:20

Log OTel init failures via ereport instead of silently swallowing

019f8ca

Add LogExporterWarning() bridge in exporter_interface.h — otel_exporter.cc cannot include postgres.h (macro conflicts with libintl.h via gRPC headers), so this bridges to ereport(WARNING) defined in stats_exporter.cc.

Copilot AI review requested due to automatic review settings April 5, 2026 21:22

iskakaushik force-pushed the refactor/exporter-cleanup branch from aadbe0b to 019f8ca Compare April 5, 2026 21:22

Copilot started reviewing on behalf of iskakaushik April 5, 2026 21:23 View session

serprex approved these changes Apr 5, 2026

View reviewed changes

Copilot AI reviewed Apr 5, 2026

View reviewed changes

src/queue/event.h Outdated Show resolved Hide resolved

Update src/queue/event.h

96f912c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 5, 2026 21:26

Copilot started reviewing on behalf of iskakaushik April 5, 2026 21:27 View session

serprex reviewed Apr 5, 2026

View reviewed changes

src/queue/event.h Outdated Show resolved Hide resolved

Update src/queue/event.h

0477f5a

Co-authored-by: Philip Dubé <159546+serprex@users.noreply.github.com>

Copilot AI reviewed Apr 5, 2026

View reviewed changes

src/export/otel_exporter.cc Show resolved Hide resolved

src/export/stats_exporter.cc Show resolved Hide resolved

fix(ci): add INET6_ADDRSTRLEN fallback and LogExporterWarning stub

309b022

- event.h: define INET6_ADDRSTRLEN (46) if not already provided, avoiding dependency on <arpa/inet.h> which is missing in some build environments - hostname_test.cc: stub LogExporterWarning to fix linker error

iskakaushik merged commit 9084f26 into main Apr 5, 2026
8 checks passed

iskakaushik deleted the refactor/exporter-cleanup branch April 5, 2026 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor exporter files: reduce noise and improve structure#54

Refactor exporter files: reduce noise and improve structure#54
iskakaushik merged 5 commits intomainfrom
refactor/exporter-cleanup

iskakaushik commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		col_db->Append(std::string(ev.datname, ev.datname_len));
		col_username->Append(std::string(ev.username, ev.username_len));

-    col_db->Append(std::string(ev.datname, ev.datname_len));
-    col_username->Append(std::string(ev.username, ev.username_len));
+    auto dlen =
+        ClampFieldLen(ev.datname_len, static_cast<uint8>(sizeof(ev.datname) - 1), "datname_len");
+    auto ulen = ClampFieldLen(ev.username_len, static_cast<uint8>(sizeof(ev.username) - 1),
+                              "username_len");
+    col_db->Append(std::string(ev.datname, dlen));
+    col_username->Append(std::string(ev.username, ulen));

Conversation

iskakaushik commented Apr 5, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants