Vai: make every func beginning transactions also roll them back

[moio]

Before we had an inconsistent practice where, in most cases, we did not roll back transactions in case of errors, which could lead to long stalling and SQLITE_BUSY errors as the DB was held hostage of unclosed transactions.

Partly addresses rancher/rancher#48384

[tomleb]

@moio I'll go back to reviewing this tomorrow but quick comment for your thoughts anyway

I'm seeing the following db queries:

• tx.Exec(): The method already rolls back on error
• s.Upsert(): The method already rolls back (well, if its an encryption error, no, but otherwise yes)
• tx.StmtExec(): The method already rolls back on error
• runAfterDelete(): Not rolling back, so need it
• runAfterUpsert(): Not rolling back, so need it
• QueryForRows(): Not rolling back, so need it

I see there are still some errors that can happen that are not rolling back the transaction. For example, I think this

lasso/pkg/cache/sql/store/store.go

Lines 270 to 278 in ccc9d15

	rows, err := s.QueryForRows(context.TODO(), txCListKeys)
	if err != nil {
	err = s.RollbackTx(txC, err)
	return err
	}
	keys, err := s.ReadStrings(rows)
	if err != nil {
	return err
	}

would need a rollback too.

I'm thinking, it's too easy to forget a Rollback call. I can see two ways of making this easier to read, let me know if you agree/disagree.

Option 1.

Remove Commit() and Cancel() from the interface TXClient. Instead, provide a method like so (go-like pseudocode):

func (dbClient DBClient) RunInTransaction(f func(txC TXClient)) error {
    txC := beginTx() // I think we can remove this from DBClient
    err := f(txC)
    if err != nil {
       rollbackErr := rollback(txC)
       // handle rollbackErr here
    }
    err := commit(txC)
    // handle commitErr here
   return nil or err or wtv
}

Now all usage within a transaction is done with RunInTransaction(). We no longer need to add a Rollback call everywhere (or forget one). I think all our usages of the DB could work this way, I don't think we're doing nested transaction or something like that (which I think could cause problem 🤔 )

Here I'd like to make it pretty much impossible for a dev to call rollback / commit multiple times. It's only called once, in one place easy to spot.

Option 2.

Another option would be something more like having BeginTx return an separate object or a function used to either commit or rollback a transaction. So the actual transaction client can only make requests within a transaction, it can't be used to commit/rollback. That forces the beginning of the transaction and the commit/rollback to be within a few lines of each other, and only once. Something like:

var err error
txC, endTx, err := dbClient.BeginTx()
defer func() {
    endErr := endTx(err)
    if endErr != nil {
        // handle commit or rollback failure
    }
}

Now visually it should be fairly easy to spot a beginTx without a endTx being called (the compiler will help too).

[tomleb]

Let's take this snippet as an example and assume the query fails.

	err = tx.Exec(createTableQuery)
	if err != nil {
		err = s.RollbackTx(tx, err)
		return nil, &db.QueryError{QueryString: createTableQuery, Err: err}
	}

tx.Exec will call rollback and return a wrapped error.

Then, the error branch is taken, so s.RollbackTx is called, which calls Cancel(), which calls Rollback again. This nested rollback should return sql.ErrTxDone because the Tx has already been canceled. So cancel returns nil, which means RollbackTx also returns nil. We end up returning a db.QueryError with a nil Err field.

Is this intended?

[moio]

No, you are right, I did not notice initially that tx.Exec rolled back internally. Fixing.

[moio]

I'm seeing the following db queries:
* tx.Exec(): The method already rolls back on error
* s.Upsert(): The method already rolls back (well, if its an encryption error, no, but otherwise yes)
* tx.StmtExec(): The method already rolls back on error
* runAfterDelete(): Not rolling back, so need it
* runAfterUpsert(): Not rolling back, so need it
* QueryForRows(): Not rolling back, so need it

All correct. And I admit not having immediately realized some of our funcs such as Exec and StmtExec deviate so much from expected behavior of the sql package without very obvious names. They are close to being footguns to me.

I see there are still some errors that can happen that are not rolling back the transaction. For example, I think this

lasso/pkg/cache/sql/store/store.go

Lines 270 to 278 in ccc9d15

	rows, err := s.QueryForRows(context.TODO(), txCListKeys)
	if err != nil {
	err = s.RollbackTx(txC, err)
	return err
	}
	keys, err := s.ReadStrings(rows)
	if err != nil {
	return err
	}

would need a rollback too.

That's correct, it escaped my attention. Added with b49e7fe

I'm thinking, it's too easy to forget a Rollback call.

Yes.

I can see two ways of making this easier to read, let me know if you agree/disagree.

Option 1.

Remove Commit() and Cancel() from the interface TXClient. Instead, provide a method like so (go-like pseudocode):

func (dbClient DBClient) RunInTransaction(f func(txC TXClient)) error {
    txC := beginTx() // I think we can remove this from DBClient
    err := f(txC)
    if err != nil {
       rollbackErr := rollback(txC)
       // handle rollbackErr here
    }
    err := commit(txC)
    // handle commitErr here
   return nil or err or wtv
}

Now all usage within a transaction is done with RunInTransaction(). We no longer need to add a Rollback call everywhere (or forget one). I think all our usages of the DB could work this way, I don't think we're doing nested transaction or something like that (which I think could cause problem 🤔 )

Nested transactions are not even supported by SQLite so we should definitely not use them. I cannot think reasons why this should not work.

Option 2. [...]

That's also possible but I prefer the functional style of the first.

That said, I would propose to still merge this PR (or a fixed version of this PR) and treat your proposal as a follow up item (your choice on whether it is a must have for 2.11 or not).

[moio]

OK, sorry, I need to take a fresh look at this one. Problem is clear, resolution not so much.

I will dedicate some time next year.