Fix deadlock with concurrent transactions

[asdine]

This fixes #298 by making sure access to the attachedTxMu mutex is only locked by Commit or Rollback if that transaction is actually attached.

[tie]

👍

Also, on lines 118–123 in database/database.go, do we need a lock if opts.Attached is false?

+ if opts.Attached {
	db.attachedTxMu.Lock()
	defer db.attachedTxMu.Unlock()

	if db.attachedTransaction != nil {
		return nil, errors.New("cannot open a transaction within a transaction")
	}
+ }

[asdine]

Also, on lines 118–123 in database/database.go, do we need a lock if opts.Attached is false?

@tie Yes because we don't want to allow to any new transaction opening when there is an attached transaction. Existing transaction may continue, but not new ones. https://github.com/genjidb/genji/blob/ac9e33be665939fcfb7c6ad02629916f0ea0faef/database/database.go#L110-L112

[tie]

We don't want to allow to any new transaction opening when there is an attached transaction.

@asdine are there any technical or practical reasons for this limitation? I don’t see how non-attached concurrent transaction tx started with BeginTx could affect attached one started with db.Exec("BEGIN"), i.e. tx.Exec("BEGIN") should return error even if there are no transactions attached to database, right?

https://github.com/genjidb/genji/blob/14c47075848a5cd4b1ca782cf9af67cb0ac069d2/sql/query/transaction.go#L16-L19

[asdine]

@tie When I started implementing this I followed SQLite behavior:

package main

import (
	"context"
	"database/sql"
	"log"

	_ "github.com/mattn/go-sqlite3"
)

func main() {
	db, err := sql.Open("sqlite3", "./foo.db")
	if err != nil {
		log.Fatal(err)
	}
	defer db.Close()

	_, err = db.Exec("CREATE TABLE IF NOT EXISTS foo(a integer)")
	if err != nil {
		panic(err)
	}

	// start t1 before BEGIN
	tx1, err := db.BeginTx(context.Background(), &sql.TxOptions{
		ReadOnly: true,
	})
	if err != nil {
		panic(err)
	}

	// start BEGIN
	_, err = db.Exec("BEGIN")
	if err != nil {
		panic(err)
	}

	// start t2 after BEGIN
	tx2, err := db.BeginTx(context.Background(), &sql.TxOptions{
		ReadOnly: true,
	})
	if err != nil {
		// panics here with: cannot start a transaction within a transaction
		panic(err)
	}

	err = tx1.Rollback()
	if err != nil {
		panic(err)
	}

	err = tx2.Rollback()
	if err != nil {
		panic(err)
	}

	_, err = db.Exec("ROLLBACK")
	if err != nil {
		panic(err)
	}
}

I think we don't have to follow that behavior if we are certain there won't be any weird edge case. I'd be happy to talk about a better design for this, but let's keep the discussion in an issue as this PR is focused on fixing the bug on the v0.8.

[tie]

We’d also need release-branch.v0.8 branch to cherry-pick this change if we want to release a patch.