Return early from allow list when possible

[camilo]

What is this?

Behaviour change:

Savepoints that don't follow the activerecord pattern active_record_savepoint_#{nesting_level} or that are nested more than two levels are not going to bypass the circuit anymore.

I think this is okay because in the end this is a best effort scenario on an overloaded database and this IS an active record adapter.

In the trilogy adapter.

We look at every single query to see wether or not it should be allowed to pass thru the circuit if it happens to be a transaction control statement, (COMMIT/ROLLBACK/RELEASE SAVEPOINT) because we want to try our hardest to make sure we don't pile open transactions onto MySQL even if the underlying database is overloaded.

Large queries, including queries with long comments (often the case when using marginalia to pass metadata around) will in aggregate use quite a bit of CPU just running regular expressions.

We can exploit the fact that active record trilogy uses a set pattern for savepoint names (`active_record_#{nesting_level}" ) and uses ROLLBACK and COMMIT without any extra characters in the end of the statement to simply return early in most queries by inspecting the last few characters.

FWIW

ruby 3.1.4p223 (2023-03-30 revision 2ec786f0d7) [x86_64-linux]
Warming up --------------------------------------
      end-with-case?   356.147k i/100ms
               regex    20.823k i/100ms
Calculating -------------------------------------
      end-with-case?      3.523M (± 1.9%) i/s -     17.807M in   5.056666s
               regex    206.105k (± 1.2%) i/s -      1.041M in   5.052247s

Comparison:
      end-with-case?:  3522827.5 i/s
               regex:   206105.0 i/s - 17.09x  slower

[adrianna-chang-shopify]

Hah, clever change 😄 I have a couple small nits, mostly on the comments. I'd also personally opt to drop the benchmark -- not sure it needs to get merged to the test suite, IMHO documenting it in this PR should and mentioning the optimization in an inline comment should suffice.

[camilo]

I'd also personally opt to drop the benchmark -- not sure it needs to get merged to the test suite, IMHO documenting it in this PR should and mentioning the optimization in an inline comment should suffice.

Only reason to keep it is because I suspect an artificial case bench that is a very short query, say SELECT * FROM table_name will not show that much improvement and then we'll have to re-explain why this works the way it works.

As it stands now there is a realistic fixture of a real world query. I don't feel strongly tho. Something else would be to add a /bench directory so we don't have a random file that does not run at test time.

---

Another discussion point but that is larger and should probably live in its own issue is the actual real life effectiveness of the bypass, the need for it, and wether or not AR could just tell us (or is already telling us?) that a query is a COMMIT/ROLLBACK/SAVEPOINT RELEASE in that case we would not have to mess with the raw queries.