Added Clone method to Txn to create an independent copy of a transaction

[feldgendler]

This PR adds a Clone method to Txn.

A subsequent PR will add similar functionality to Txn in memdb (txn.Snapshot returning a read-only transaction), dependent on this one.

[hashicorp-cla]

All committers have signed the CLA.

[banks]

@feldgendler thanks for the PR!

Can you share a bit more about what you are trying to achieve with this? iradix and memdb by extension are inherently single-writer systems. You can't have a copy-on-write radix tree that can be independently modified by multiple writers without totally changing the design to be lock free etc.

It could be possible to safely clone a read-only transaction in MemDB, but iradix has no notion of read-only transactions since transactions are only ever used for mutations. So I'm not sure this method is ever going to be correct. It would be possible to add a Clone method to memdb transactions provided that it always returned a read-only transaction (even if called on a read-write transaction) and so only ever preserved the snapshot of the current transation and not any of it's mutable state. That means you'd never need to clone internal iradix transactions to build it.

[feldgendler]

We are already using forked memdb and iradix, and my plan was to create a PR for memdb as soon as this one is merged.

This is our memdb fork: https://github.com/ridge/go-memdb

What we need is indeed what you say: a memdb.Txn.Clone method that always returns a read-only transaction even if called on a read-write one. It takes a snapshot of the current transaction. That's what we need.

But I don't see a way to implement memdb.Txn.Clone without first introducing Clone to iradix. Do you?

[banks]

Got it. The part I was missing is that the RO clone of a RW transaction still needs to see the writes that have already taken place in that transaction so far. I was assuming since the MemDB would be read only it wouldn't need a write set or any table-level transactions at all, just the old memdb root.

I think that's OK and this implementation is good but would you be able to update the Clone function docs to make this design more explicit - it seems important that users would need to understand the semantics.

[banks]

Actually, I'm not sure it is OK.

Currently iradix assumes that any radix nodes it has already written (in the LRU cache) in the transaction are not yet visible to any other thread and so mutates them in place. Once you add this clone method even if the other thread never attempts to mutate (which isn't prevented by the API) it can still race since the writing thread modifies the new radix nodes in place and so a reader following the root node might hit a node bing mutated in another thread.

The only way to do this safely would be effectively to Commit the transaction and get a root pointer for the new snapshot including current writes, share that as the "read-only" copy, and then reset the write transaction (clear LRU of modified nodes, reset snapshot and root to the new snapshot) to point to the intermediate snapshot and track further mutations.

Have you tried running your fork concurrently with -race detector on?

[feldgendler]

would you be able to update the Clone function docs to make this design more explicit

Not sure what I should write there. The cloned iradix transaction is as good as the original. Both can be used independently to produce new immutable trees.

Currently iradix assumes that any radix nodes it has written in the transaction are not yet visible to any other thread.

That's why I clear t.writable in Clone. I was indeed running into concurrency issues until I realized I need to do that. With t.writable cleared, writeNode will treat all existing nodes as immutable, and won't attempt to write into them.

Have you tried running your fork concurrently with -race detector on?

Yes, our fork is a central part of a project that's tested extensively with -race enabled.

[banks]

Thanks for bearing with me through a few context switches... you're right this does reset the LRU cache which means that it's only copy ing pointers to radix tress which won't be mutated further by either transaction.

I have a suggestion for the doc comment - if you agree I'll add that before merge.

[feldgendler]

Thanks a lot! I'll now submit a PR for memdb to complete the loop.

[feldgendler]

Updated PR hashicorp/go-memdb#66, ready to merge.