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

feat: write to UUID mapper and relation tuples in one SQL transaction #1340

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

alnr
Copy link
Collaborator

@alnr alnr commented Jun 13, 2023

This wraps an SQL transaction around the UUID mapper's and the relation tuple manager's write operations.

Previously, we would insert into the UUID mapping table outside the SQL transaction where we updated/inserted the relation tuples. This is now one transaction.

Beside the obvious benefit of the whole operation being atomic, this improves performance in CockroachDB global tables, where each write operation outside of a transaction takes at least the time to commit the implicit transaction (100++ ms). Inside a transaction, inserts are faster and we have to pay the commit latency only once.

I'm open suggestions on how to better cut across the abstraction layers (handler vs manager/persister) 🤷 .

Before:

image

After:
image

Note: in these traces, the COMMIT-equivalent of CRDB is the sql-conn-exec before the sql-tx-commit span.

@alnr alnr self-assigned this Jun 13, 2023
@alnr alnr requested review from zepatrik and hperl as code owners June 13, 2023 15:56
Copy link
Collaborator

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I really like how you implemented this :).

So if I understood correctly, every time we want to do things e.g. from a handler, we now request a Transactor, which wraps the closure in a transaction.

@alnr
Copy link
Collaborator Author

alnr commented Jun 15, 2023

LGTM! I really like how you implemented this :).

Thanks ;)

So if I understood correctly, every time we want to do things e.g. from a handler, we now request a Transactor, which wraps the closure in a transaction.

Yep, pretty much. For now, I've changed onlyTransactTuples. Maybe there are more instances where this approach makes sense?

@alnr alnr added this pull request to the merge queue Jun 15, 2023
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

Merged via the queue into master with commit eeeecf6 Jun 15, 2023
23 checks passed
@alnr alnr deleted the transact-tuples branch June 15, 2023 10:06
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

Successfully merging this pull request may close these issues.

None yet

3 participants