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

Add retry support for serializable transactions #386

Merged

Conversation

neekolas
Copy link
Collaborator

@neekolas neekolas commented Apr 30, 2024

tl;dr

I noticed our libxmtp test suite was occasionally failing due to serializable transaction errors. This adds retries, which are required when using the serializable isolation level since errors are common and transient thanks to Postgres' conflict detection

Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite Graphite

@neekolas neekolas marked this pull request as ready for review April 30, 2024 14:37
@neekolas neekolas requested review from 37ng and insipx April 30, 2024 14:39
@neekolas neekolas force-pushed the 04-30-add_retry_support_for_serializable_transactions branch from c65d2bc to 4c59544 Compare April 30, 2024 15:01
var err error
for i := 0; i < numRetries; i++ {
select {
case <-ctx.Done():
Copy link
Contributor

@insipx insipx Apr 30, 2024

Choose a reason for hiding this comment

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

noob Q: What does this select on this ctx do?

The default case returns nil when it finishes succesfully, but ctx seems like some sort of global context? When would it trigger?

Copy link

@37ng 37ng Apr 30, 2024

Choose a reason for hiding this comment

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

If you ctrl+c or the context is out of time, then ctx.Done() channel will receive a struct and this select block will return the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. It just stops you from continuing retries when the server is in the process of shutting down

@neekolas neekolas merged commit 827b3f2 into main Apr 30, 2024
4 checks passed
@neekolas neekolas deleted the 04-30-add_retry_support_for_serializable_transactions branch April 30, 2024 16:39
Copy link
Collaborator Author

The specific tests that were failing are a little worrying to me because they all used random wallets and inboxes, and executed each test serially, so they should be isolated from one another.

We may have to do some debugging around exactly what Postgres considers a "conflict" in the specific queries we are using here. Maybe it's a broader set of rows than you'd expect.

But these retries are a generally good idea regardless and we can tackle that separately.

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.

3 participants