- 
                Notifications
    You must be signed in to change notification settings 
- Fork 130
feat: add retries to sql macros #2529
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: add retries to sql macros #2529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Adds SQL query retry mechanism with exponential backoff and improves memory efficiency across the codebase.
- Added retry logic in sql_query_macros.rswith 100ms base retry and max 16 retries
- Removed MaxSqlRetrieserror variant fromWorkflowErrorin favor of macro-level retry handling
- Renamed querytotxnin database drivers for better semantic clarity
- Added reference operators (&) to SQL query parameters across multiple services to prevent unnecessary cloning
- Simplified transaction retry logic to use consistent backoff for all database errors
12 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| if is_retrying.load(Ordering::Relaxed) { | ||
| self.query(|| async { | ||
| sql_execute!( | ||
| [self, &pool] | ||
| " | ||
| DELETE FROM workflow_signal_events | ||
| WHERE location = jsonb(?1) | ||
| ", | ||
| location, | ||
| ) | ||
| .await | ||
| }) | ||
| sql_execute!( | ||
| [self, &pool] | ||
| " | ||
| DELETE FROM workflow_signal_events | ||
| WHERE location = jsonb(?1) | ||
| ", | ||
| location, | ||
| ) | ||
| .await | ||
| .map_err(|x| fdb::FdbBindingError::CustomError(x.into()))?; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Potential race condition: the DELETE operation should be in the same transaction as the INSERT to maintain atomicity
| let mut backoff = $crate::__rivet_util::Backoff::new( | ||
| 4, | ||
| None, | ||
| $crate::utils::sql_query_macros::QUERY_RETRY_MS, | ||
| 50 | ||
| ); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Backoff parameters are inconsistent - base=4 and factor=50 here vs QUERY_RETRY_MS=100 defined at the top. Consider aligning these values or documenting why they differ.
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | ||
| | WorkerCrashed => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding RowNotFound to non-retryable errors since retrying won't help if the row doesn't exist
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| | WorkerCrashed => { | |
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| | WorkerCrashed | RowNotFound => { | 
| Deploying rivet with   | 
| Latest commit: | c307626 | 
| Status: | ✅ Deploy successful! | 
| Preview URL: | https://758a1d78.rivet.pages.dev | 
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet.pages.dev | 
200c987    to
    b3eae4a      
    Compare
  
    | Deploying rivet-studio with   | 
| Latest commit: | c307626 | 
| Status: | ✅ Deploy successful! | 
| Preview URL: | https://bb342d1e.rivet-studio.pages.dev | 
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-studio.pages.dev | 
| Deploying rivet-hub with   | 
| Latest commit: | c307626 | 
| Status: | ✅ Deploy successful! | 
| Preview URL: | https://6498ef2a.rivet-hub-7jb.pages.dev | 
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-hub-7jb.pages.dev | 
5e33d41    to
    7666132      
    Compare
  
    b3eae4a    to
    4fc5f51      
    Compare
  
    4fc5f51    to
    406d6f9      
    Compare
  
    7666132    to
    20eace2      
    Compare
  
    20eace2    to
    b77a826      
    Compare
  
    406d6f9    to
    d44c0b9      
    Compare
  
    d44c0b9    to
    c307626      
    Compare
  
    b77a826    to
    863f139      
    Compare
  
    | Merge activity
 | 
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->


Changes