-
-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sub-transactions API #912
base: develop
Are you sure you want to change the base?
Sub-transactions API #912
Conversation
d2412fe
to
eba8dc6
Compare
Solution: implement an interface for sub-transactions It's a fairly simplistic interface at this point and it allows choosing a mode of release on drop (commit by default). This is an improved version of the original implementation in pgx-contrib-spiext (https://github.com/supabase/pgx-contrib-spiext/blob/main/src/subtxn.rs)
Any feedback I can get on this from the maintainers? Thanks! |
pgx/src/subxact.rs
Outdated
// This allows SubTransaction to be de-referenced to SpiClient | ||
impl<'conn, const COMMIT: bool> Deref for SubTransaction<SpiClient<'conn>, COMMIT> { | ||
type Target = SpiClient<'conn>; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
self.parent.as_ref().unwrap() | ||
} | ||
} | ||
|
||
// This allows a SubTransaction of a SubTransaction to be de-referenced to SpiClient | ||
impl<Parent: SubTransactionExt, const COMMIT: bool> Deref | ||
for SubTransaction<SubTransaction<Parent>, COMMIT> | ||
{ | ||
type Target = Parent; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
self.parent.as_ref().and_then(|p| p.parent.as_ref()).unwrap() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more general for SubTransaction<T, COMMIT>
to deref into T
? Is there a problem with that approach? Too much recursion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it was initially https://github.com/supabase/pgx-contrib-spiext/blob/main/src/subtxn.rs#L137-L143
However, this has prevented me from using SubTransaction<T: Deref<SpiClient>>
so I had to resort to boxing: https://github.com/supabase/pgx-contrib-spiext/blob/develop/src/subtxn.rs#L169-L178
However, with this special case, I no longer need a Box
pgx/src/subxact.rs
Outdated
/// Sub-transaction | ||
/// | ||
/// Can be created by calling `SpiClient::sub_transaction`, `SubTransaction<Parent>::sub_transaction` | ||
/// or any other implementation of `SubTransactionExt` and obtaining it as an argument to the provided closure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My internal grammarian is trying to say the function names should be subtransaction
(which is valid in English), whereas the type names should be SubTransaction
, but that seems... somehow peculiar, given that normally those would normalize to either sub_transaction
and SubTransaction
like you have it here or subtransaction
and Subtransaction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named it sub_transaction
for consistency with SubTransaction
. But ultimately I don't have a strong opinion about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And Postgres calls them "SubTransactions" in their API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that the type/trait names should use SubTransaction
. Maybe I will start using sub_transaction
in anger and it will bug me enough to want to take the _
out, and I'll just let it be inconsistent-but-natural then. But otherwise it seems fine.
pgx/src/subxact.rs
Outdated
// The reason we are not calling this `released` as we're also using this flag when | ||
// we convert between commit_on_drop and rollback_on_drop to ensure it doesn't get released | ||
// on the drop of the original value. | ||
should_release: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"on the drop of the original value"... hm.
pgx/src/subxact.rs
Outdated
fn into(mut self) -> SubTransaction<Parent, false> { | ||
let result = SubTransaction { | ||
memory_context: self.memory_context, | ||
resource_owner: self.resource_owner, | ||
should_release: self.should_release, | ||
parent: self.parent.take(), | ||
}; | ||
// Make sure original sub-transaction won't commit | ||
self.should_release = false; | ||
result | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. We copy everything into a new SubTransaction and then let Drop
run for the previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Does this resolve #912 (comment) or do you want to provide additional feedback on this?
It may be unnecessary. Solution: simply derive it
Solution: name it Parent as it is what it is
Also, given `PgMemoryContexts` API is still somewhat rough, it may be not a great idea to expose it. Solution: remove it
…is for Solution: comment it
Solution: provide some reference materials on it
It is only there so that we can move it out of manually copied type that implements Drop Solution: extract release logic into a separate trait
Solution: document them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general this is okay. I've never used any of Postgres subtransaction facilities from C before, and haven't studied their docs, so I'm leaning heavily on the thought that you have.
That said, before we can merge I'd like to see two things...
-
A lot of additional doc comment documentation in
subxact.rs
, from the module-level down. I think this needs some exposition around not only what it is, but how it interacts with the database, and how that might impact the user's code and design decisions. -
A new example that demonstrates using this, maybe in some kind of fabricated scenario that users could maybe adapt to their own purposes (ie, something a little more polished than, say, our "strings" example). And with that example, a nice README.md that covers topics around proper subxact handling.
This code itself isn't very complex, which is nice. But it's a brand new feature and I'd like it to not only be as immediately useful as possible to users, but also serve as an example of what a new feature PR ought to look like. ;)
Other Thoughts
- I also wonder if/how this should interop with pgx' existing subact hooks support (see
callbacks.rs/register_subxact_callback()
and friends. - What needs to be said in docs (or done in code?) with regards to
PgTryBuilder
and subtransactions? - Is this also usable as-is by background workers? Is there an opportunity for expanding the existing bgworker example to show off subxact support?
- I'm sure I'll come up with other stuff as we get this merged.
pgx/src/prelude.rs
Outdated
@@ -47,3 +47,6 @@ pub use crate::pg_sys::{ | |||
check_for_interrupts, debug1, debug2, debug3, debug4, debug5, ereport, error, function_name, | |||
info, log, notice, warning, FATAL, PANIC, | |||
}; | |||
|
|||
// Sub-transactions | |||
pub use crate::subxact::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to limit this to just the module: pub use crate::subxact;
.
I think we're trying to be a little better about globbing everything into the current namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the best. At the very least, I think we should consider pub use crate::subxact::{self, SubTransactionExt}
for the sub-transaction functionality to be easily available.
// Remember the memory context before starting the sub-transaction | ||
let ctx = PgMemoryContexts::CurrentMemoryContext.value(); | ||
// Remember resource owner before starting the sub-transaction | ||
let resource_owner = unsafe { pg_sys::CurrentResourceOwner }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jubilee means when might Postgres initially set (or later change) this and how are we certain it even has a valid value at this point in time?
Thank you for your feedback.
|
Updated subtransactions to work on recent develop. I will now work on docs/examples and other concerns listed above. |
We can do better Solution: export only most common types
@yrashk are you still planning on taking this over the finish line? |
Yes, I've discovered some issues with the API that make its usability reduced (mostly related to mutability) when porting my "contrib" crate to use this core and extending it to essentially allow for error recovery without losing the entire subtransaction stack. I'm working on rethinking this and attempting to see what I need to change in the core of this PR to make this possible. |
Fair enough. Subxacts are definitely pretty hard to reason about. |
Problem: inability to fine-tune transaction boundaries easily
Solution: implement an interface for sub-transactions
It's a fairly simplistic interface at this point, allowing choosing a mode of release on drop (commit by default).
This is an improved version of the original implementation in pgx-contrib-spiext
(https://github.com/supabase/pgx-contrib-spiext/blob/main/src/subtxn.rs)
I welcome feedback as the interface is somewhat opinionated (however, I tried to make it as palatable as possible, and its previous iteration in pgx-contrib-spiext went through a few rounds of improvements to be used in real extensions). I've tried to follow our recent work on
SpiClient
in spirit.It'd be great to get additional scrutiny for the safety and soundness of the implementation.
A message to pgx users: my ability to actively contribute to pgx largely depends on your support. Please consider sponsoring my work