prisma-engines icon indicating copy to clipboard operation
prisma-engines copied to clipboard

fix: remove actor system for transactions

Open laplab opened this issue 1 year ago • 2 comments
trafficstars

Closes https://github.com/prisma/team-orm/issues/1219

laplab avatar Jul 01 '24 12:07 laplab

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.050MiB 2.057MiB -7.005KiB
Postgres (gzip) 817.279KiB 820.453KiB -3.174KiB
Mysql 2.019MiB 2.027MiB -8.780KiB
Mysql (gzip) 804.657KiB 808.035KiB -3.378KiB
Sqlite 1.915MiB 1.919MiB -3.804KiB
Sqlite (gzip) 764.001KiB 766.277KiB -2.276KiB

github-actions[bot] avatar Jul 01 '24 12:07 github-actions[bot]

CodSpeed Performance Report

Merging #4940 will not alter performance

Comparing feat/remove-actors-attempt-2 (4146626) with main (c4fe130)

Summary

✅ 11 untouched benchmarks

codspeed-hq[bot] avatar Jul 02 '24 14:07 codspeed-hq[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 09 '24 11:07 CLAassistant

I think I have found a bug after testing with smash. Since we are only dropping the connection from the transactions hashmap in the manager after the timeout has passed, the connection we took from the pool lingers for the timeout period, making future transactions wait for connections longer than necessary.

Update: This is fixed now.

laplab avatar Jul 09 '24 12:07 laplab

Just so it won't get lost in Slack threads. Right now it breaks tracing tests in prisma/prisma link. Looks like the reason for that is that itx_query_builder span got renamed into itx_execute_single. New name is much better, but it's sort of a breaking change for tracing feature - span is user-facing. It's fine to do it, tracing is still in preview, we just need to be conscious and explicit about it.

SevInf avatar Jul 09 '24 16:07 SevInf

cc: @millsp @aqrln see this comment from @SevInf: https://github.com/prisma/prisma-engines/pull/4940#issuecomment-2218199242

I just read "breaking change for Tracing feature" just to keep it in mind in case it's relevant.

apolanc avatar Jul 12 '24 07:07 apolanc

I've found the reason why spans are missing and it's a doozy!

Reportedly missing spans are actually emitted by the query engine, but they lack the parentSpanId property. The only thing they have is a link to the operation that they are related to. For example, span with INSERT statement will have a link to the create() operation.

parentSpanId is missing because previously the tokio task running the transaction was responsible for setting it and it is no longer there. Moreover the sequence of spans related to the transactions was completely separate from the query operations. On the same level as create() and findMany() operations, we had an itx_runner span, which had all interactive transactions operations under it.

Now that the interactive transaction operations are executed in the same task as the create() and findMany() operations, we find ourselves in the world where itx_execute_single spans are under their respective operations, but the commit and rollback operations are still under itx_runner. There is also some logic to derive trace_id from transaction id, which might be obsolete now - I need to double-check it.

In any case, it looks like the actor system isn't going to give up without a fight until the very end!

laplab avatar Jul 16 '24 11:07 laplab