From 5816f6157beec58d1df60dc00e31168999fd235e Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 13 May 2022 13:46:30 -0700 Subject: [PATCH 1/2] Stop asserting in apparently multithreaded cases This assertion is misleading: if the Rust runtime becomes nested, it creates a "new" thread, without actually spawning a second thread. This debug_assert! then yields erroneous assertions. --- pgx-pg-sys/src/submodules/guard.rs | 11 ----------- pgx-pg-sys/src/submodules/setjmp.rs | 10 +++------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/pgx-pg-sys/src/submodules/guard.rs b/pgx-pg-sys/src/submodules/guard.rs index fe384b36f..6877ee190 100644 --- a/pgx-pg-sys/src/submodules/guard.rs +++ b/pgx-pg-sys/src/submodules/guard.rs @@ -76,17 +76,6 @@ fn take_panic_location() -> PanicLocation { thread_local! { pub(crate) static IS_MAIN_THREAD: once_cell::sync::OnceCell<()> = once_cell::sync::OnceCell::new() } pub fn register_pg_guard_panic_handler() { - // first, lets ensure we're not calling ourselves twice - #[cfg(debug_assertions)] - { - if IS_MAIN_THREAD.with(|v| v.get().is_some()) { - panic!("IS_MAIN_THREAD has already been set") - } - - // it's expected that this function will only ever be called by `pg_module_magic!()` by the main thread - IS_MAIN_THREAD.with(|v| v.set(()).expect("failed to set main thread sentinel")); - } - std::panic::set_hook(Box::new(|info| { #[cfg(debug_assertions)] { diff --git a/pgx-pg-sys/src/submodules/setjmp.rs b/pgx-pg-sys/src/submodules/setjmp.rs index d55df6c56..845d0e20b 100644 --- a/pgx-pg-sys/src/submodules/setjmp.rs +++ b/pgx-pg-sys/src/submodules/setjmp.rs @@ -7,17 +7,13 @@ /// /// Currently, this function is only used by pgx' generated Postgres bindings. It is not (yet) /// intended (or even necessary) for normal user code. +/// +/// Calling this function from anything but the main thread can result in unpredictable behavior. #[inline(always)] pub(crate) unsafe fn pg_guard_ffi_boundary T>(f: F) -> T { use crate as pg_sys; - // as the panic message says, we can't call Postgres functions from threads - // the value of IS_MAIN_THREAD gets set through the pg_module_magic!() macro - #[cfg(debug_assertions)] - if crate::submodules::guard::IS_MAIN_THREAD.with(|v| v.get().is_none()) { - panic!("functions under #[pg_guard] cannot be called from threads"); - }; - + // This should really, really not be done if IS_MAIN_THREAD is None. let prev_exception_stack = pg_sys::PG_exception_stack; let prev_error_context_stack = pg_sys::error_context_stack; let mut jump_buffer = std::mem::MaybeUninit::uninit(); From 44ef9bbf4d260e55183791f0de8adf0bd5152564 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Mon, 16 May 2022 11:49:09 -0700 Subject: [PATCH 2/2] Remove all IS_MAIN_THREAD code --- pgx-pg-sys/src/submodules/guard.rs | 20 -------------------- pgx-pg-sys/src/submodules/setjmp.rs | 2 +- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/pgx-pg-sys/src/submodules/guard.rs b/pgx-pg-sys/src/submodules/guard.rs index 6877ee190..3cb0d3265 100644 --- a/pgx-pg-sys/src/submodules/guard.rs +++ b/pgx-pg-sys/src/submodules/guard.rs @@ -70,28 +70,8 @@ fn take_panic_location() -> PanicLocation { }) } -// via pg_module_magic!() this gets set to Some(()) for the "main" thread, and remains at None -// for all other threads. -#[cfg(debug_assertions)] -thread_local! { pub(crate) static IS_MAIN_THREAD: once_cell::sync::OnceCell<()> = once_cell::sync::OnceCell::new() } - pub fn register_pg_guard_panic_handler() { std::panic::set_hook(Box::new(|info| { - #[cfg(debug_assertions)] - { - if IS_MAIN_THREAD.with(|v| v.get().is_none()) { - // a thread that isn't the main thread panic!()d - // we make a best effort to push a message to stderr, which hopefully - // Postgres is logging somewhere - eprintln!( - "thread={:?}, id={:?}, {}", - std::thread::current().name(), - std::thread::current().id(), - info - ); - } - } - PANIC_LOCATION.with(|p| { let existing = p.take(); diff --git a/pgx-pg-sys/src/submodules/setjmp.rs b/pgx-pg-sys/src/submodules/setjmp.rs index 845d0e20b..4679e1b24 100644 --- a/pgx-pg-sys/src/submodules/setjmp.rs +++ b/pgx-pg-sys/src/submodules/setjmp.rs @@ -13,7 +13,7 @@ pub(crate) unsafe fn pg_guard_ffi_boundary T>(f: F) -> T { use crate as pg_sys; - // This should really, really not be done if IS_MAIN_THREAD is None. + // This should really, really not be done in a multithreaded context let prev_exception_stack = pg_sys::PG_exception_stack; let prev_error_context_stack = pg_sys::error_context_stack; let mut jump_buffer = std::mem::MaybeUninit::uninit();