C++ exception barriers: remaining C-ABI getters by JoshDreamland · Pull Request #94 · ClickHouse/pg_stat_ch

JoshDreamland · 2026-05-11T22:27:35Z

Summary

C++ exception safety cleanup for src/export/stats_exporter.cc. Three commits:

b4a4181 — more exception barriers (@serprex, amended). Same content as more exception barriers #87 with the function-try-block syntax restored to brace form, and PschExporterInit's catch handlers switched from reset + return false (latent NPE on the bgworker's next loop) to ereport(FATAL) (clean proc_exit(1); postmaster respawns the worker without DB-wide crash recovery).
a7d2ea9 — fix: round out exception barriers. Wrap the remaining unguarded extern "C" getters (PschGetConsecutiveFailures, PschGetRetryDelayMs, PschResetRetryState) in try / catch. Drop a load-bearing comment in ExportEventsAsArrowInternal and PschExportBatch marking the live-RAII boundary above the post-export PG accounting calls. Cache NumConsecutiveFailures(); tighten the barrier comment.
16bd888 — feat: install C++ terminate handler. Backstop for anything the explicit barriers miss. Installed at the top of PschExporterInit (the build lock from build: forbid C++ sources outside src/export/ #68 makes _PG_init C-only, so it can't host std::set_terminate). Converts uncaught C++ throws into ereport(FATAL) → proc_exit(1) so postmaster respawns just our bgworker instead of treating SIGABRT as a backend crash. The explicit catches still run first; this only fires for what they miss.

Test plan

CI green (GCC matrix + Clang job).

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR rounds out the C++ exception-barrier audit for src/export/stats_exporter.cc, ensuring extern "C" entry points don’t allow C++ exceptions to escape into PostgreSQL C frames, and documenting a key longjmp/RAII boundary around post-export accounting.

Changes:

Converted PschExporterInit / PschExportBatch to brace-form try { ... } catch { ... } exception barriers.
Added exception barriers to remaining C-ABI retry/getter functions (PschGetConsecutiveFailures, PschGetRetryDelayMs, PschResetRetryState) plus a null guard for PschGetConsecutiveFailures.
Added “RAII must not cross longjmp-capable PG calls” invariant comments near post-export accounting points.

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

Copilot

Pull request overview

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

serprex · 2026-05-12T05:48:20Z

+// Exception barrier: exporter destructors (clickhouse-cpp socket close, gRPC
+// stub teardown, protobuf arena release) can throw. Catching here prevents the
+// throw from crossing the on_proc_exit chain.
 void PschExporterShutdown(void) {
-  g_exporter.exporter.reset();
+  try {
+    g_exporter.exporter.reset();
+  } catch (const std::bad_alloc&) {
+    LogExporterWarning("exporter shutdown", "out of memory");
+  } catch (const std::exception& e) {
+    LogExporterWarning("exporter shutdown exception", e.what());
+  }


can we use https://en.cppreference.com/cpp/error/terminate_handler to achieve same effect as elog FATAL?

Really good call; sorry I didn't think of it after Claude explained to me the circumstances under which Postmaster throws a tantrum. Makes a lot of sense. I think these explicit catches should stay, though—the fatal ones because they provide more accurate logging and more explicitly demonstrate intent, and the non-fatal ones because they're non-fatal and properly rescue the process.

Copilot

Pull request overview

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

+  // Install the C++ terminate handler before any other C++ work runs in this
+  // process.  See PschTerminateHandler above for the rationale.  Idempotent —
+  // a second call in the same process would just reinstall the same handler.
+  std::set_terminate(PschTerminateHandler);
+


+jq --arg v "$VERSION" \
+  '.version = $v | .provides.pg_stat_ch.version = $v' \
+  "$META" > "$tmp"
+mv "$tmp" "$META"
+echo "updated $META -> $VERSION"


iskakaushik

good change!

Add try / catch wrappers to the three extern "C" entry points that were missing them: PschGetConsecutiveFailures, PschGetRetryDelayMs, PschResetRetryState. The virtual methods they call are trivial integer accesses today, but they're part of the StatsExporter interface; any future override that allocates could throw across the bgworker's PG_TRY frame. Match the pattern used by the surrounding init/batch/shutdown entries: log, return a safe default (0 for the int getters, no-op for the void resetter). Also add a null-pointer guard to PschGetConsecutiveFailures so it matches its sister getters. Mark the live-RAII boundary in ExportEventsAsArrowInternal and PschExportBatch where the ArrowBatchBuilder / std::vector<PschEvent> are still on the stack above the post-export PG accounting calls (pg_atomic_fetch_add_u64, PschRecordExportSuccess, RecordExporterFailure). None of those PG calls longjmp today (LWLockAcquire is not interruptible; atomics are pure), so this is not a current bug — but a future longjmping call there would skip C++ destructors and leak the Arrow/protobuf/ZSTD buffers (heap, not palloc). Load-bearing comment instead of restructure: tells anyone adding a PG call here to scope the RAII tighter first. Small polish from review: - PschExportBatch's barrier comment now describes the C/C++ boundary rather than the specific PG_TRY frame (more accurate vs how the hazard actually surfaces). - PschGetRetryDelayMs caches NumConsecutiveFailures() once instead of reading the same atomic three times. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Anything the explicit barriers in stats_exporter.cc miss would default to std::terminate -> abort -> SIGABRT, which the postmaster treats as a backend crash and uses to trigger DB-wide crash recovery (every connection severed, WAL replayed). Route uncaught C++ exceptions through ereport(FATAL) instead so they become clean proc_exit(1) calls: postmaster respawns just our bgworker on its bgw_restart_time. Installation lives inside PschExporterInit rather than _PG_init. After the plugin layer's port to C99 (#88), _PG_init is a C function and cannot call std::set_terminate; and the bgworker is the only process that ever runs our C++ exception-throwing code paths, so installing in its first C++ entry point is both necessary and sufficient. This is a backstop, not a replacement. The explicit catches in PschExportBatch / ExportEventStats / ExportEventsAsArrow still run first and let us survive an allocation hiccup without losing the exporter. PschExporterInit's FATAL catch is also preserved for its specific, informative error message — the terminate handler would otherwise log a generic "uncaught bad_alloc". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings May 11, 2026 22:27

Copilot started reviewing on behalf of JoshDreamland May 11, 2026 22:28 View session

Copilot AI reviewed May 11, 2026

View reviewed changes

Comment thread src/export/stats_exporter.cc

Comment thread src/export/stats_exporter.cc Outdated

Comment thread src/export/stats_exporter.cc Outdated

JoshDreamland force-pushed the exception-barrier-cleanup branch from 0a14144 to 077ac22 Compare May 11, 2026 22:39

Copilot AI review requested due to automatic review settings May 12, 2026 02:44

Copilot started reviewing on behalf of JoshDreamland May 12, 2026 02:45 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

serprex requested review from iskakaushik and serprex May 12, 2026 05:26

serprex mentioned this pull request May 12, 2026

more exception barriers #87

Closed

serprex approved these changes May 12, 2026

View reviewed changes

JoshDreamland mentioned this pull request May 12, 2026

build: forbid C++ sources outside src/export/ #68

Merged

4 tasks

Copilot AI review requested due to automatic review settings May 12, 2026 20:27

JoshDreamland force-pushed the exception-barrier-cleanup branch from 94d250e to 0ce0477 Compare May 12, 2026 20:27

JoshDreamland changed the base branch from main to port-hook-path-to-c May 12, 2026 20:27

Copilot started reviewing on behalf of JoshDreamland May 12, 2026 20:27 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

JoshDreamland force-pushed the port-hook-path-to-c branch from fde97e5 to eb7cc35 Compare May 12, 2026 21:05

JoshDreamland force-pushed the exception-barrier-cleanup branch from 0ce0477 to 4d2214d Compare May 12, 2026 21:07

JoshDreamland force-pushed the port-hook-path-to-c branch from eb7cc35 to 5566554 Compare May 12, 2026 21:44

JoshDreamland force-pushed the exception-barrier-cleanup branch from 4d2214d to 3e64292 Compare May 12, 2026 21:44

iskakaushik approved these changes May 12, 2026

View reviewed changes

JoshDreamland force-pushed the port-hook-path-to-c branch from 5566554 to 0cb3ee0 Compare May 13, 2026 01:12

JoshDreamland force-pushed the exception-barrier-cleanup branch from 3e64292 to fb45223 Compare May 13, 2026 01:12

JoshDreamland force-pushed the port-hook-path-to-c branch 2 times, most recently from 3a6176c to 369b69d Compare May 13, 2026 01:26

JoshDreamland force-pushed the exception-barrier-cleanup branch from fb45223 to 7b83369 Compare May 13, 2026 01:26

JoshDreamland force-pushed the port-hook-path-to-c branch from 369b69d to 68d74e5 Compare May 13, 2026 02:05

JoshDreamland force-pushed the exception-barrier-cleanup branch from 7b83369 to 8ea8b36 Compare May 13, 2026 02:05

JoshDreamland changed the title ~~C++ exception barriers: brace-form + remaining C-ABI getters~~ C++ exception barriers: remaining C-ABI getters May 13, 2026

Base automatically changed from port-hook-path-to-c to main May 13, 2026 14:45

serprex and others added 3 commits May 13, 2026 10:49

more exception barriers

b4a4181

JoshDreamland force-pushed the exception-barrier-cleanup branch from 8ea8b36 to 16bd888 Compare May 13, 2026 14:49

JoshDreamland merged commit a1c1aa9 into main May 13, 2026
19 checks passed

JoshDreamland deleted the exception-barrier-cleanup branch May 13, 2026 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ exception barriers: remaining C-ABI getters#94

C++ exception barriers: remaining C-ABI getters#94
JoshDreamland merged 3 commits into
mainfrom
exception-barrier-cleanup

JoshDreamland commented May 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

serprex May 12, 2026

Uh oh!

JoshDreamland May 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

iskakaushik left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Summary

Test plan

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

serprex May 12, 2026

Choose a reason for hiding this comment

Uh oh!

JoshDreamland May 12, 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!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoshDreamland commented May 11, 2026 •

edited

Loading