Capture Rust backtraces for Postgres ERRORs raised through pg_sys FFIcalls (pgcentralfoundation#2262)

When Rust code calls a pg_sys function (e.g. pg_sys::relation_open())
and that function internally raises an ERROR via elog/ereport, the
longjmp is caught by pg_guard_ffi_boundary and converted to a Rust
panic. The panic hook captures a Rust backtrace at that point, but it
was never attached to the error — the CaughtError::PostgresError path
went through pg_re_throw() which bypassed do_ereport() entirely, so the
backtrace was discarded.

Fix this by:

- In `downcast_panic_payload()`, when unwrapping a
`CaughtError::PostgresError`, attach the backtrace from PANIC_LOCATION
to the error report. This is the common path used by both `PgTryBuilder`
and `run_guarded`, so the backtrace is available regardless of how the
error is caught.

- In `run_guarded(),` route CaughtError::PostgresError through
GuardAction::Report (and thus do_ereport) instead of
GuardAction::ReThrow (and pg_re_throw). This ensures the backtrace
appears in the ERROR's DETAIL line when the error propagates all the way
out.

- In `pg_guard_ffi_boundary`, call `FlushErrorState(`) after`
CopyErrorData` to properly clean the original error off Postgres'
fixed-size errordata[] stack before do_ereport pushes a new entry.

- Remove the now-unused `GuardAction::ReThrow` variant and
`pg_re_throw()` call.

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>

Author: eeeebbbbrrrr

pgrx-pg-sys/src/submodules/ffi.rs

@@ -240,9 +240,12 @@ unsafe fn pg_guard_ffi_boundary_impl<T, F: FnOnce() -> T>(f: F) -> T {
             };
             let line = errdata.lineno as _;
 
-            // clean up after ourselves by freeing the result of [CopyErrorData] and restoring
-            // Postgres' understanding of where its next longjmp should go
+            // clean up after ourselves by freeing the result of [CopyErrorData], flushing the
+            // Postgres error state (so the original error no longer occupies a slot on Postgres'
+            // fixed-size errordata[] stack), and restoring Postgres' understanding of where its
+            // next longjmp should go
             pg_sys::FreeErrorData(errdata_ptr);
+            pg_sys::FlushErrorState();
             pg_sys::PG_exception_stack = prev_exception_stack;
             pg_sys::error_context_stack = prev_error_context_stack;
 

pgrx-pg-sys/src/submodules/panic.rs

@@ -343,17 +343,20 @@ pub fn register_pg_guard_panic_hook() {
 
     let default_hook = std::panic::take_hook();
     std::panic::set_hook(Box::new(move |info: _| {
-        if is_os_main_thread() == Some(true) {
-            // if this is the main thread, swallow the panic message and use postgres' error-reporting mechanism.
-            PANIC_LOCATION.with(|thread_local| {
-                thread_local.replace({
-                    let mut info: ErrorReportLocation = info.into();
-                    info.backtrace = Some(std::backtrace::Backtrace::capture());
-                    Some(info)
-                })
-            });
-        } else {
-            // if this isn't the main thread, we don't know which connection to associate the panic with.
+        // Always capture the backtrace into PANIC_LOCATION so that
+        // downcast_panic_payload can attach it to CaughtError::PostgresError.
+        // This is a thread-local, so it's harmless on non-main threads.
+        PANIC_LOCATION.with(|thread_local| {
+            thread_local.replace({
+                let mut info: ErrorReportLocation = info.into();
+                info.backtrace = Some(std::backtrace::Backtrace::capture());
+                Some(info)
+            })
+        });
+
+        if is_os_main_thread() == Some(false) {
+            // definitely not the main thread -- we don't know which connection
+            // to associate the panic with, so also call the default hook
             default_hook(info)
         }
     }))
@@ -386,7 +389,6 @@ impl CaughtError {
 #[derive(Debug)]
 enum GuardAction<R> {
     Return(R),
-    ReThrow,
     Report(ErrorReportWithLevel),
 }
 
@@ -428,16 +430,6 @@ where
 {
     match unsafe { run_guarded(AssertUnwindSafe(f)) } {
         GuardAction::Return(r) => r,
-        GuardAction::ReThrow => {
-            #[cfg_attr(target_os = "windows", link(name = "postgres"))]
-            unsafe extern "C-unwind" {
-                fn pg_re_throw() -> !;
-            }
-            unsafe {
-                crate::CurrentMemoryContext = crate::ErrorContext;
-                pg_re_throw()
-            }
-        }
         GuardAction::Report(ereport) => {
             do_ereport(ereport);
             unreachable!("pgrx reported a CaughtError that wasn't raised at ERROR or above");
@@ -454,10 +446,11 @@ where
     match catch_unwind(f) {
         Ok(v) => GuardAction::Return(v),
         Err(e) => match downcast_panic_payload(e) {
-            CaughtError::PostgresError(_) => {
-                // Return to the caller to rethrow -- we can't do it here
-                // since we this function's has non-POF frames.
-                GuardAction::ReThrow
+            CaughtError::PostgresError(ereport) => {
+                // Postgres raised this error via longjmp, which pg_guard_ffi_boundary caught
+                // and converted into a Rust panic.  downcast_panic_payload already attached the
+                // Rust backtrace from the panic hook, so just report it through do_ereport.
+                GuardAction::Report(ereport)
             }
             CaughtError::ErrorReport(ereport) | CaughtError::RustPanic { ereport, .. } => {
                 GuardAction::Report(ereport)
@@ -470,7 +463,20 @@ where
 pub(crate) fn downcast_panic_payload(e: Box<dyn Any + Send>) -> CaughtError {
     if e.downcast_ref::<CaughtError>().is_some() {
         // caught a previously caught CaughtError that is being rethrown
-        *e.downcast::<CaughtError>().unwrap()
+        let mut caught = *e.downcast::<CaughtError>().unwrap();
+
+        // For PostgresErrors (originating from a pg_sys FFI longjmp caught by
+        // pg_guard_ffi_boundary), the panic hook captured a Rust backtrace into
+        // PANIC_LOCATION.  Attach it now so callers (PgTryBuilder, run_guarded,
+        // etc.) can include it in the ERROR's DETAIL line.
+        if let CaughtError::PostgresError(ref mut ereport) = caught {
+            if ereport.inner.location.backtrace.is_none() {
+                let panic_location = take_panic_location();
+                ereport.inner.location.backtrace = panic_location.backtrace;
+            }
+        }
+
+        caught
     } else if e.downcast_ref::<ErrorReportWithLevel>().is_some() {
         // someone called `panic_any(ErrorReportWithLevel)`
         CaughtError::ErrorReport(*e.downcast().unwrap())

pgrx-tests/src/tests/pg_try_tests.rs

@@ -66,6 +66,67 @@ mod tests {
         Spi::get_one::<()>("SELECT crash()").map(|_| ())
     }
 
+    /// When a `pg_sys::` FFI call raises a Postgres ERROR (via longjmp), the error should
+    /// carry a Rust backtrace so that it appears in the ERROR's DETAIL line.
+    #[pg_test]
+    fn test_postgres_error_contains_backtrace() {
+        use pgrx::pg_sys::panic::CaughtError;
+
+        let has_backtrace = PgTryBuilder::new(|| {
+            // relation_open with a bogus OID will raise an ERROR inside Postgres
+            unsafe {
+                pg_sys::relation_open(pg_sys::Oid::INVALID, pg_sys::AccessShareLock as _);
+            }
+            unreachable!("relation_open should have raised an ERROR");
+        })
+        .catch_others(|e| match e {
+            CaughtError::PostgresError(ref ereport) => ereport.backtrace().is_some(),
+            _ => panic!("expected CaughtError::PostgresError, got {e:?}"),
+        })
+        .execute();
+
+        assert!(has_backtrace, "PostgresError from a pg_sys FFI call should contain a backtrace");
+    }
+
+    /// When a Postgres ERROR propagates through Rust frames, destructors must run (via panic
+    /// unwinding) AND the resulting error should carry a Rust backtrace in its detail.
+    #[pg_test]
+    fn test_postgres_error_runs_drop_and_has_backtrace() {
+        use pgrx::pg_sys::panic::CaughtError;
+
+        let dropped = Rc::new(AtomicBool::new(false));
+
+        struct Guard(Rc<AtomicBool>);
+        impl Drop for Guard {
+            fn drop(&mut self) {
+                self.0.store(true, Ordering::SeqCst);
+            }
+        }
+
+        let (drop_ran, has_backtrace) = PgTryBuilder::new(|| {
+            let _guard = Guard(dropped.clone());
+
+            // relation_open with a bogus OID raises an ERROR inside Postgres, which
+            // pg_guard_ffi_boundary converts to a panic.  The panic unwinds through
+            // this frame, running Guard's destructor.
+            unsafe {
+                pg_sys::relation_open(pg_sys::Oid::INVALID, pg_sys::AccessShareLock as _);
+            }
+            unreachable!("relation_open should have raised an ERROR");
+        })
+        .catch_others(|e| {
+            let bt = match e {
+                CaughtError::PostgresError(ref ereport) => ereport.backtrace().is_some(),
+                _ => panic!("expected CaughtError::PostgresError, got {e:?}"),
+            };
+            (dropped.load(Ordering::SeqCst), bt)
+        })
+        .execute();
+
+        assert!(drop_ran, "Guard's Drop impl should have run during panic unwinding");
+        assert!(has_backtrace, "PostgresError should carry a Rust backtrace");
+    }
+
     #[pg_test]
     fn test_pg_try_execute_no_error_no_catch() {
         let result = PgTryBuilder::new(|| 42).execute();