tcdi
`pg_try` is a no-op in trusted plrust
It seems that the #[cfg(feature="postgrestd")] implementation of pg_try turns it into a no-op, which seems to be done so the guard doesn't inject additional setjmp calls (for performance reasons, as I understand it).
But pg_try can still be called directly in safe code, so making it a no-op is confusing.
For trusted plrust, we can't allow pg_try to actually work, so I suppose we should block it?
What would make sense to me is if pgx had a feature to control error handling (maybe error_abort or something), and a separate feature (maybe trusted_mode) that would disable things like PANIC and pg_try that we can't allow in a trusted PL. Making the trusted-ness rely on the error behavior seems confusing to me, unless I'm missing something.
Hm? pg_try interacts with PgLogLevel::PANIC? I mean, if a PANIC happens, sure, I think that would catch it, but otherwise I was pretty sure it only interacted with Rust panic!, which aborts the transaction in PL/Rust, and in PGX may throw an ereport. The reason it is converted into a no-op is to simplify the guard code because if Postgres throws an ERROR, then everything inside PL/Rust is simply going to be shut down and the Rust runtime is going to say "oh well, I'm dead now" and lie down quietly and die, and that's Fine, Actually, because it's equivalent to "destructors are not always run", and as you noted, it isn't necessarily desirable to have PL/Rust catch error states when Postgres decides to abort the transaction.
So to clarify:
It seems that the
#[cfg(feature="postgrestd")]implementation ofpg_tryturns it into a no-op, which seems to be done so the guard doesn't inject additionalsetjmpcalls (for performance reasons, as I understand it).
No, correctness reasons. Very convoluted ones, but nonetheless.
I still don't see why pg_try should be a no-op in plrust. It certainly surprises me as a user. It can't be made safe in trusted plrust, so I would expect it to be rejected at compile time (ideally) or runtime if the pg_try is ever reached (regardless of whether an ERROR is thrown) with a message saying that pg_try is not allowed.
pg_try interacts with PgLogLevel::PANIC?
No. I was just using those as two examples of things that can't be allowed by trusted plrust. So, if we had, for example, a feature for the pgx crate called trusted_mode, it would have to reject both of those.
Whereas the abort-on-error behavior seems like a separate thing to me that might be desirable separately without trusted mode.
I shouldn't have brought up multiple points in one issue, because I think it's distracting from the main point that I find the pg_try behavior surprising and I think it should be rejected instead.
No, correctness reasons. Very convoluted ones, but nonetheless.
I understand that pg_try is certainly not something that can be in a trusted PL (unless it was implemented with subtransactions like in plpgsql).
But for the specific case of turning C longjmp() into rust panic!(), I don't immediately see why that can't be done in trusted plrust correctly.
I thought based on our discord conversation that the main reason was performance of the additional setjmp calls. There's also the issue that catching a longjmp in rust is relying on UB, but that would be the same for other pgx usage.
In my work on the Rust standard library, I have become very comfortable with merging, splitting, or removing feature flags entirely. I am not attached to a fixed suite of feature flags, and for under-explored features which may not have sharp definitions, I am comfortable naming them after the project they are associated with. I have no intention of altering the way these feature flags are segmented at this time as I do not wish to become more attached to features currently in-flux by trying to move them to more sensical or "permanent" names.
As for pg_try, the way it was changed is because an implementation detail that is far deeper inside pgx, and is in many cases called directly, was changed. I don't really intend to focus on addressing higher-level niceties at this time.
I don't really intend to focus on addressing higher-level niceties at this time.
By that you mean that pg_try will remain a no-op in plrust?
I can't say that I agree, but so be it.
https://github.com/tcdi/pgx/pull/834 will completely replace pg_try and its implementation, effectively mooting this discussion.