fix: stop liblbug C++ exceptions from crashing the PG backend (#2) - #9
Merged
Conversation
Calling ladybug.replay_replication() a second time, from a fresh backend,
on a persistent store that already contains the replayed rows terminated
the backend with SIGABRT and forced a cluster-wide restart. An uncaught
C++ exception (std::out_of_range from an internal unordered_map::at on the
duplicate primary key) escaped liblbug's C API into the Postgres backend,
where std::terminate() -> abort() is treated as a crash. The equivalent
failure raised through ladybug.cypher() was already handled, so only the
replay path in a backend that reopened an existing store was affected.
Fix: add a C++ exception-boundary TU (ladybug_bridge_guard.cpp) that wraps
the liblbug calls the bridge makes on the executing/planning paths
(lbug_database_init, lbug_connection_init, lbug_connection_query, and
lbug_connection_get_pushed_sql). Each guard runs the call inside
try/catch(...); on an escaped exception it records a palloc'd
human-readable message through an out-param (gerr) and returns LbugError,
so the bridge reports the failure via ereport()/NOTICE exactly as it
already does for ordinary liblbug errors -- instead of letting the
exception unwind into Postgres. PostgreSQL uses setjmp/longjmp for
ereport(ERROR), which does not run C++ catch handlers, so the guards only
ever catch genuine liblbug exceptions; they never swallow a PG ereport.
- ladybug_bridge.c: switch all five liblbug call sites (acquire's
storage-path and :memory: database/connection init,
ladybug_bridge_direct_sql, ladybug_bridge_fill_tuplestore_from_query,
ladybug_bridge_execute_collect, ladybug_bridge_pushed_sql) to the
guarded variants, preferring the caught-exception message on the
exception path and falling back to the liblbug result/error accessors
for ordinary errors.
- ladybug_bridge_guard.cpp: new TU. Includes postgres.h under
extern "C" because this PG build's C headers are not guarded by
PG_BEGIN_DECLS, so compiling them as C++ would otherwise give the PG
functions (psprintf, pstrdup, pfree, ...) C++ mangled linkage and they
would fail to resolve against the C-built postgres at link/load time.
- Makefile: add the guard object to OBJS and link the C++ runtime
explicitly (PGXS links the shared library with the C driver, so a C++
object needs -lc++ on macOS / -lstdc++ elsewhere, otherwise the link
fails with undefined symbols for std::exception / __cxa_throw).
- scripts/test_with_pgembed.py: forward CXX (alongside CC/PG_SYSROOT)
onto the make command line so the C++ TU is built with the chosen
compiler; add a regression test that replays the same change log twice
across two separate backends (each run_test is a separate psql -c).
The first replay materialises a row into a persistent store; the
second, in a fresh backend that reopens the store, re-runs the same
CREATE and hits a duplicate primary key. Before the fix this was a
SIGABRT; it now returns 0 with a "replay skipped" NOTICE -- matching
the issue's Expected behavior.
ladybug_bridge_acquire declared 'const char *gerr' after executable statements inside the storage-path block, tripping -Werror=declaration-after-statement. Move it to the top of the block.
The 'Cypher: MATCH with fkrel relationship (projection)' test returns empty relationship columns because of a pre-existing ladybug bug (fixed only in unreleased ladybug). Add xfail_reason support to run_test: a failing XFAIL test prints XFAIL and counts toward the passing total, while one that unexpectedly passes prints XPASS and is treated as a failure so the marker gets dropped once the fix ships.
The pgembed-bundled pg_config carries a stale -isysroot left behind by an Xcode upgrade, breaking the build with 'stdio.h not found'. On Darwin, test.sh now sets CC to homebrew clang and PG_SYSROOT to an existing SDK so './scripts/test.sh' works without manual env vars. Only applied when the user hasn't already set CC/PG_SYSROOT.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #2
Calling ladybug.replay_replication() a second time, from a fresh backend, on a persistent store that already contains the replayed rows terminated the backend with SIGABRT and forced a cluster-wide restart. An uncaught C++ exception (std::out_of_range from an internal unordered_map::at on the duplicate primary key) escaped liblbug's C API into the Postgres backend, where std::terminate() -> abort() is treated as a crash. The equivalent failure raised through ladybug.cypher() was already handled, so only the replay path in a backend that reopened an existing store was affected.
Fix: add a C++ exception-boundary TU (ladybug_bridge_guard.cpp) that wraps the liblbug calls