Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Subtransactions tracking/preventions #46

Open
palkan opened this issue Jun 10, 2022 · 2 comments
Open

Subtransactions tracking/preventions #46

palkan opened this issue Jun 10, 2022 · 2 comments
Assignees
@palkan
Labels
enhancement New feature or request

Comments

@palkan
Copy link
Owner

palkan commented Jun 10, 2022 •
edited

Is your feature request related to a problem? Please describe.

https://about.gitlab.com/blog/2021/09/29/why-we-spent-the-last-month-eliminating-postgresql-subtransactions/

Describe the solution you'd like

It would be great if Isolator could track subtransactions and report them.
The feature MUST be optional (for now):

Isolator.configure do |config|
  # Must be set to a non-zero value to be activated (so, default is 0)
  config.max_substransactions_depth = 32
end

Additional context

Gitlab implementation: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67918/diffs

Rails discussion: rails/rails#44518
@palkan palkan added the enhancement New feature or request label Jun 10, 2022
@palkan palkan self-assigned this Jun 10, 2022
@composerinteralia
Copy link

composerinteralia commented Aug 31, 2023 •
edited

It would also be great if we could track queries or transactions to a different database cluster. Stuff like:

Cluster1Model.transaction do
  Cluster2Model.transaction do
  #...
  end
end

composerinteralia added a commit to composerinteralia/rails that referenced this issue Sep 7, 2023
@composerinteralia @ipc103
Instrument Active Record transactions
229b66d 
Tracking Active Record-managed transactions seems to be a common need,
but there's currently not a great way to do it. Here's a few examples
I've seen:

* GitHub has custom transaction tracking that monkey patches the Active
  Record `TransactionManager` and `RealTransaction`. We use the tracking
  to prevent opening a transaction to one database cluster inside a
  transaction to a different database cluster, and to report slow
  transactions (we get slow transaction data directly from MySQL as well,
  but it's still helpful to report from the application with backtraces to
  help track them down).
* https://github.com/palkan/isolator tracks transactions to prevent
  non-atomic interactions like external network calls inside a
  transaction. The gem works by subscribing to `sql.active_record`, then
  piecing together the transactions by looking for `BEGIN`, `COMMIT`,
  `SAVEPOINT`, etc., but this is unreliable:
    - palkan/isolator#65
    - palkan/isolator#64
* It looks like GitLab patches `TransactionManager` and `RealTransaction`
  to track nested savepoints. See palkan/isolator#46

This commit adds a new `transaction.active_record` event that should
provide a more reliable solution for these various use cases. It
includes the connection in the payload (useful, for example, in
differentiating transactions to different databases), but if this change
gets merged we're also planning to add details about what type of
transaction it is (savepoint or real) and what the outcome is (commit,
rollback, restarted, errored).

This instrumentation needs to start and finish at fairly specific times:
  - start on materialize
  - finish after committing or rolling back, but before the after_commit
    or after_rollback callbacks
  - finish and start again when the transaction restarts (at least for
    real transactions—we've done it for savepoints as well but I'm not
    certain we should)
  - ensure it finishes if commit and rollback fail (e.g. if the
    connection goes away)
To make all that work, this commit uses the lower-level `#build-handle`
API instead of `#instrument`.

Co-authored-by: Ian Candy <ipc103@github.com>
@ipc103 ipc103 mentioned this issue Sep 7, 2023
Merged
4 tasks
@tagirahmad tagirahmad mentioned this issue Sep 11, 2023
Open
3 tasks
ipc103 added a commit to ipc103/rails that referenced this issue Sep 12, 2023
@composerinteralia @ipc103
Instrument Active Record transactions
ce49fa9 
Tracking Active Record-managed transactions seems to be a common need,
but there's currently not a great way to do it. Here's a few examples
I've seen:

* GitHub has custom transaction tracking that monkey patches the Active
  Record `TransactionManager` and `RealTransaction`. We use the tracking
  to prevent opening a transaction to one database cluster inside a
  transaction to a different database cluster, and to report slow
  transactions (we get slow transaction data directly from MySQL as well,
  but it's still helpful to report from the application with backtraces to
  help track them down).
* https://github.com/palkan/isolator tracks transactions to prevent
  non-atomic interactions like external network calls inside a
  transaction. The gem works by subscribing to `sql.active_record`, then
  piecing together the transactions by looking for `BEGIN`, `COMMIT`,
  `SAVEPOINT`, etc., but this is unreliable:
    - palkan/isolator#65
    - palkan/isolator#64
* It looks like GitLab patches `TransactionManager` and `RealTransaction`
  to track nested savepoints. See palkan/isolator#46

This commit adds a new `transaction.active_record` event that should
provide a more reliable solution for these various use cases. It
includes the connection in the payload (useful, for example, in
differentiating transactions to different databases), but if this change
gets merged we're also planning to add details about what type of
transaction it is (savepoint or real) and what the outcome is (commit,
rollback, restarted, errored).

This instrumentation needs to start and finish at fairly specific times:
  - start on materialize
  - finish after committing or rolling back, but before the after_commit
    or after_rollback callbacks
  - finish and start again when the transaction restarts (at least for
    real transactions—we've done it for savepoints as well but I'm not
    certain we should)
  - ensure it finishes if commit and rollback fail (e.g. if the
    connection goes away)
To make all that work, this commit uses the lower-level `#build-handle`
API instead of `#instrument`.

Co-authored-by: Ian Candy <ipc103@github.com>
@palkan
Copy link
Owner Author

palkan commented Oct 21, 2023

Hey @composerinteralia

It would also be great if we could track queries or transactions to a different database cluster

Added here 7824716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@palkan palkan

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@palkan @composerinteralia