Combine Tx() & RWTx()

[benbjohnson]

The transaction API is currently:

func (db *DB) Tx() (*Tx, error)
func (db *DB) RWTx() (*Tx, error)

That made sense when there were separate transaction types (Transaction & RWTransaction) but now there's just one.

The new API would look like:

func (db *DB) Tx(writable bool) (*Tx, error)

This lines up better with the types and the underlying Tx.Writable() function.

/cc @tv42

[pkorotkov]

@benbjohnson
Sounds sensible, I'd however use read(-only) version for the flag. A concrete meaning leaves no chance to get entangled even looking at the func signature :) Just an opinion :)

[benbjohnson]

LMDB uses readOnly as the flag but I find it confusing that I'm passing in a positive value (true) in order to not do something (e.g. write). Ultimately, it's kind of a moot point since most people will use DB.Do() and DB.With(). I wish there were better function names for those two functions but I haven't thought of anything better yet.

[pkorotkov]

@benbjohnson
Please don't get me wrong, the code like this

tx, err := db.Tx(false)

might also confuse if a user neglects package documentation :) Otherwise, he/she knows what that boolean means. In such a case-being extremely concerned about the purity of readability- it's probably better to let both Tx and RWTx further exist.

[tv42]

Alternative for Do & With: DB.Read(func ...) and DB.Write(func ...)?
I am personally having a hard time remembering which one is which, of Do and With.

I still feel like I would gain from having separate types for read-only and writable transactions, with functions expecting former also able to accept the latter. But I'm not going to argue that much louder.

[tv42]

I do like the mirroring of writable bool vs Tx.Writable() bool.

[benbjohnson]

@tv42 I did some more research in the database/sql package and what do you think of this interface:

func (db *DB) Begin(writable bool) (*Tx, error)

That would fit more closely with the database/sql implementation:

func (db *DB) Begin() (*Tx, error)

That would also free up the Tx() function name which could be used for the transactional blocks. Either something like this:

func (db *DB) Tx(func(*Tx) error) error
func (db *DB) RWTx(func(*Tx) error) error

Or combine them like this:

func (db *DB) Tx(writable bool, func(*Tx) error) error

The first form is more explicit but the second form is more inline with Begin(writable bool). What do you think about that?

I hear what you're saying about the read-only and read-write transaction types but it becomes a huge headache to implement something higher level like an ORM.

[tv42]

SQL carries baggage all the way from the times of transactions staying open waiting for user to complete a form. I'm not a huge fan of imitating that world too closely, though I understand your motivation to make things look familiar.

Whether it's called Tx or Begin doesn't really make a huge difference in my eyes.

I'll give DB.Begin() a slight negative score from my point of view, because it violates this rule I have: operations that strongly hint at being part of a series (start/stop, open/close, begin/end) should reside on the same layer. But then even the typical OO way of using files breaks that rule, with (*os.File).Close. So the rule ain't perfect ;) But in that sense, "give me a transaction" "commit this transaction" make things flow better, inside my head. So Tx, or something like that, wins.

I'm not a fan of ORMs either ;)

I think db.Tx(false, fn) is a worse API than db.With(fn). I'm just not thrilled about the bool. I don't care as much about it for Tx/Begin, because it seems all my code is using With/Do -- the error handling is so much easier, there. Using With/Do, I feel like I have some hope of learning/remembering which is which, with that bool I feel I'll make more copy-paste style mistakes. Tx/RWTx would be an improvement, or the Read/Write I suggested earlier -- no way I can ever confuse those.

Examples that I find completely acceptable:

func (db *DB) Begin(writable bool) (*Tx, error)
func (db *DB) Tx(func(*Tx) error) error
func (db *DB) RWTx(func(*Tx) error) error

func (db *DB) Tx(writable bool) (*Tx, error)
func (db *DB) Read(func(*Tx) error) error
func (db *DB) Write(func(*Tx) error) error

func (db *DB) Tx() (*Tx, error)
func (db *DB) RWTx() (*Tx, error)
func (db *DB) Read(func(*Tx) error) error
func (db *DB) Write(func(*Tx) error) error

func (db *DB) Tx(writable bool) (*Tx, error)
func (db *DB) Snapshot(func(*Tx) error) error
func (db *DB) Mutate(func(*Tx) error) error

... and now that I've typed those out -- this might be confusing:

db.RWTx(func (tx *bolt.Tx) error {
  // make changes here
  tx.Commit()
  return errors.New("mwahaha")
})

is that an indication that the API could be better? That Commit/Rollback belong in a layer that isn't passed into the function?

[benbjohnson]

Thanks for the extensive feedback. I'm going to sleep on it. Nothing is jumping out at me yet. I like that Read/Write is so explicit but my biggest issue with that is that it conflicts with the Reader/Writer interfaces which are so common in Go.

Right now I'm leaning toward Begin/Tx/RWTx. It somewhat matches database/sql and it avoids the writable boolean flag.

I don't quite understand the last comment about the Commit and error return. Are you thinking that Commit/Rollback should be inside the block? I like it because it lets me error and rollback in one and I can check my application errors and database errors in one:

err := db.RWTx(func (tx *bolt.Tx) error {
  return tx.Bucket("widgets").Put([]byte("foo"), []byte("bar"))
})
if err != nil {
  return err
}

[tv42]

One more name suggestion came to mind for the With/Do Read/Write Snapshot/Mutate list: View/Edit.

I like the Snapshot/Mutate and View/Edit ones because they kinda remind you that you're actually looking at a snapshot/view of the database, and holding it open costs resources. They also are more engaging to me than Tx/RWTx, and whenever I see Do I was already thinking Do Edit The Database.

P.S. I don't think the docs currently say that boltdb does snapshot isolation -- that is, read-only transactions see a snapshot of the db as it was when they started. That is how I think it works, though, and that would make sense ;)

[tv42]

Those Read/Write don't implement io.Reader/Writer anyway, because the types are different. So Go itself won't be confused, though the programmer might.

I really like the Tx/View/Edit names. Whether that's Tx(writable) or Tx/RWTx doesn't make a huge difference to me; I like Tx/RWTx a little bit more, but your ORM use case might benefit from a flag.

As for the example at the end, I firmly believe Commit/Rollback belongs in the With/Do level, I'm not suggesting a change there. But as it is now, the inner function can call Commit/Rollback, and the result can be confusing. That's what the example tried to demonstrate. In the example, the changes are committed to the db, but the caller sees an error. Imagine that Commit being somewhere a few levels deeper, and it can get messy to debug. I don't immediately see a use case for the inner function being able to call Commit/Rollback.

Sorry for multiple threads of conversation.

[benbjohnson]

Ok, I mulled it over and I'm liking this verbiage:

func (db *DB) Begin(writable bool) (*Tx, error)
func (db *DB) View(func(*Tx) error) error
func (db *DB) Update(func(*Tx) error) error

Reasons:

• They're all verbs.
• DB.Tx() is confusing since there's also a Tx type.
• DB.Begin() is similar to the database/sql package.
• I feel like Update() sounds more database-y than Edit().

[tv42]

I like that.