Remove dubious thread assertion #562
Conversation
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.
|
These are only debug asserts to begin with, so they only work if they are part of testing, but: not without completely revising the way we approach this, and probably not in a way that we would reasonably consider "trustworthy". That is, we could simply check if we share the identity of a Rust main thread. But this seems easily forged, so it would be more security theater than security wall. |
pgx-pg-sys/src/submodules/guard.rs
Outdated
| @@ -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() } | |||
There was a problem hiding this comment.
Seems we should remove this too.
There was a problem hiding this comment.
I was slightly more hesitant to remove all of that machinery because it seemed more possibly-useful. But if you are confident that it is okay to remove, I will happily do so!
Yeah, this was an ill-considered thing from the beginning. The intention was just to let the programmer know they used a Postgres-internal function in a background thread. Ya know, just to be "helpful". As we work towards the "postgrestd" stuff, I'm imagining that |
|
I imagine we might offer the facilities for naming the current thread, but probably not spawning one unless we can somehow make sure things go through a properly reentrant interface. |
workingjubilee
dismissed
eeeebbbbrrrr’s stale review
Eric is currently out, and his comments were addressed. I will happily followup on anything needing fixing.
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.