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

sql: use a closure to wrap transactions #469

Merged
merged 15 commits into from
Feb 5, 2025

Conversation

moio
Copy link
Contributor

@moio moio commented Jan 30, 2025

This introduces the a WithTransaction function, which is then used for all transactional work in Steve.

Example:

err := s.WithTransaction(ctx, true, func(tx transaction.Client) error {
		// do things with transaction such as
		_, err := tx.Exec(createTableQuery)
		if err != nil {
			return &db.QueryError{QueryString: createTableQuery, Err: err}
		}
		return nil
	})

Because WithTransaction takes care of all Begins, Commits and Rollbacks, it eliminates the problem where forgotten open transactions can block all other operations (with long stalling and SQLITE_BUSY errors).

This also:

  • merges together the disparate DBClient interfaces in one only db.Client interface with one unexported non-test implementation. I found this much easier to follow
  • refactors the transaction package in order to make it as minimal as possible, and as close to the wrapped sql.Tx and sql.Stmt functions as possible, in order to reduce cognitive load when working with this part of the codebase
  • simplifies tests accordingly
  • adds a couple of known files to .gitignore

Partly addresses rancher/rancher#48384

Best reviewed commit-by-commit - the vast majority of the changes are actually mechanical. The core of this change is "just" commit 8d7272e

Credits to @tomleb for suggesting the approach.

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Just some cosmetic suggestions -- makes sense when I walk through the commits as recommended, and looks good - making this straightforward. And I didn't think we were always rolling back on failures, in some cases we were leaving transactions dangling. Which I guess might work sometimes....

@moio
Copy link
Contributor Author

moio commented Feb 3, 2025

And I didn't think we were always rolling back on failures, in some cases we were leaving transactions dangling. Which I guess might work sometimes....

Right, that's the whole point of this PR - we had cases in which dangling transactions blocked new ones. This should really prevent that from happening!

moio added 15 commits February 3, 2025 11:47
…ent and sqlcache.informer.factory.DBClient

Signed-off-by: Silvio Moioli <[email protected]>
…ent and sqlcache.db.DBClient

Signed-off-by: Silvio Moioli <[email protected]>
…ent and sqlcache.informer.DBClient

Signed-off-by: Silvio Moioli <[email protected]>
Use the exported interface exclusively in other packages.

Signed-off-by: Silvio Moioli <[email protected]>
… exported interface in other packages)

Signed-off-by: Silvio Moioli <[email protected]>
This introduces the db.Client.WithTransaction func, which is used for
all transactional work in Steve.

This function makes sure either Commit or Rollback is called after
Beginning a transaction, thereby avoiding cases when a forgotten
open transaction can block all other operations.

This commit also heavily refactors transaction.Client and associated
code in order to make it as minimal and as close to the wrapped
sql.Tx functions as possble, in order to reduce cognitive load
when working with this part of the codebase.

Signed-off-by: Silvio Moioli <[email protected]>
Note that some tests did not make sense any more after the refactoring,
therefore those were dropped.

Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
@moio moio force-pushed the vai_transaction_refactoring branch from 31c03f8 to 349ab35 Compare February 3, 2025 10:53
@moio moio requested a review from ericpromislow February 3, 2025 10:53
@moio
Copy link
Contributor Author

moio commented Feb 3, 2025

Rebased on top of current main and force pushed.

Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Looks great, I like how you hid the Commit() / Rollback() functionality to ensure no one is calling it.

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

LGTM

@moio moio merged commit 772dc75 into rancher:main Feb 5, 2025
1 check passed
@moio moio deleted the vai_transaction_refactoring branch February 5, 2025 09:05
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