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.

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

Don't use CockroachDB transactions in Tracelistener #783

Open
Pitasi opened this issue May 13, 2022 · 4 comments
Open

Don't use CockroachDB transactions in Tracelistener #783

Pitasi opened this issue May 13, 2022 · 4 comments

Comments

@Pitasi
Copy link
Contributor

Pitasi commented May 13, 2022

My assumption from what I can tell from the tracelistener code is this:

  • we run transactions with a single INSERT statement inside (most of the time*)

And we do that by calling this emeris-utils method: https://github.com/EmerisHQ/emeris-utils/blob/main/database/database.go#L58.

Transactions are way more expensive than simple INSERT so we probably should avoid them if they are not really necessary.

Full docs is here: https://www.cockroachlabs.com/docs/stable/transactions.html.


* there is a limit on how many params a single query can have. That's why we have this "split" logic in place:
https://github.com/EmerisHQ/tracelistener/blob/2a8bed11a512a52039bc6bf1a157672fdcedb03a/tracelistener/tracelistener.go#L140-L142

This takes a single "WritebackOp" (= an INSERT or DELETE query) and splits it into multiple chunked queries.

I can't tell how often this happen. It is possible that some blocks have so many data in it that requires multiple queries but even in these cases I'm not sure a transaction is truly needed.

If it is, we can have an if that runs a transaction only for these multiple-queries cases, while having the happy path without transaction.

DoD

  • decide/discuss if we really need transactions for when a trace requires multiple INSERTs
  • avoid doing transactions when not necessary, just run the query directly
@tbruyelle
Copy link
Contributor

I modified the test TestWritebackOp_ChunkingWorks which inserts a lot of rows to compare with or without transactions.

I updated the number of inserts to 500.000, and added a small timer around each insert loop (one loop with tx, and one loop without). On my machine the result are clear:

TX  4.457864967s
NO TX  2.44812082s

The only benefit of having this transaction as I understood is only the retry on error, but I have no idea if this affects TL or not.

@Pitasi
Copy link
Contributor Author

Pitasi commented Jun 1, 2022

The only benefit of having this transaction as I understood is only the retry on error

The "retry on errors" of the cockroachdb-go library only concerns transactions, no transactions = no errors = no need to retry 😁

@tbruyelle
Copy link
Contributor

Ok then let's go :)

@tbruyelle
Copy link
Contributor

If it is, we can have an if that runs a transaction only for these multiple-queries cases, while having the happy path without transaction.

Even in that case, I don't believe transactions are required because multi-queries data are totally unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants