Add support for TxnFeature for all standard datastore implementations

[Wondertan]

Txn provides atomicity, unlike Batches as per the interface definition. However, every provided implementation only supports Batches.

[welcome]

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

• "Priority" labels will show how urgent this is for the team.
• "Status" labels will show if this is ready to be worked on, blocked, or in progress.
• "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

[gammazero]

Not every underlying datastore supports transactions, and introducing transaction functionality for a datastore that does not provide that may not be practical.

@Wondertan Are you asking for transaction functionality to be build on top of data stores like pebble and S3? Or, are you asking that all datastore implementations support the TxnFeature interface, and for those datastore implementations that do not support transactions a call to NewTransaction returns a not supported error?

[Wondertan]

@gammazero, the latter. More specifically for basic and sync datastores to support it. Badger and pebble seem to support them already, but all the auxiliary implementations in this repo do not.

[gammazero]

IMO, only datastores that have support for transactions should implement the TxnFeature interface. I do not like calling NewTransaction to get and error returned. That should mean that the caller expected to create a transaction, but there was a failure to do so. If a datastore does not support transactions, then the TxnFeature interface should not be implemented. This makes it very clear in the code that a datastore does not support transactions, versus it failed to created one.

Unless there is a strong case for non-transaction datastores to implement TnxFeature, then I I would prefer to close this issue.

[Wondertan]

If a datastore does not support transactions, then the TxnFeature interface should not be implemented.

Of course, if it doesn't support it, there is no point in returning a meaningless error; the issue is a request to add such support.

Right now, basic_ds supports BatchFeature, while it doesn't support TxnFeature . Similarly, sync_ds wrapper supports BatchFeature, but it doesn't support TxnFeature. One can compose a thread-safe batching datastore from sync and basic_ds, but can't compose a transactional one.

My particular use case is testing. I use the above to mock a real datastore (e.g. badger). Now that I have to ensure atomicity, I can't use Batch, but Transaction. However, txns are, surprisingly, not provided out of the box.

Considering how this repository has always been a toolbox for every datastore interface use case, it's a valid issue to have and eventually implement.

[gammazero]

Thank you for the clarification

We could create a TxnMapDatastore that would implement the Txn interface on top of a map datastore. A simple implementation could be essentially a thread-safe batching implementation with an additional Discard function, but that does not model the transactional behavior of any particular datastore (isolation levels, lock granularity, etc.) Should a txn query operate on an older version of the data, should it block write operations, should different writes block or fail on commit?

The point is that implementing transactional behavior for one of a provided basic datastore does not model the transactional behavior of any other particular datastore. This means that it is not a reliable way to test transactions for anything but it's own implementation. Tests that work with the TxnMapDatastore may fail in different ways with other transactional datadstores, and it is dangerous to assume otherwise.

Therefore, it seems safer to not provide a transaction capable implementation of a basic datastore, and instead make the test author test transactions using their intended datastore, or create a mock that simulates that datastore's transactional behavior the is of interest to the test.

[Wondertan]

The point is that implementing transactional behavior for one of a provided basic datastore does not model the transactional behavior of any other particular datastore.

One can reliably rely on behaviors defined by the interface with a test suite asserting those. Implementations then implement those behaviors and use the test suite to confirm they follow the contract. Then, you can confidently say that two different implementations model each others behaviors, because they follow the same interface with the same test suite confirming that. Yes, they may have completely different implementations and with major differences in behaviors, but you do know for sure that behaviors you care about are consistent amongst them with a proof.

In my case, guarantee's defined in the Transaction contract are sufficient, although it doesn't seem to have sufficient coverage in this repo.

but that does not model the transactional behavior of any particular datastore (isolation levels, lock granularity, etc.) Should a txn query operate on an older version of the data, should it block write operations, should different writes block or fail on commit?

From my understanding, all of the questions are answered by the interface contract. Batch is not transactional because when its being written to it can start writing to the underlying datastore without waiting for the final Commit call. Transaction on the other hand will accumulate amends and flush them altogether once Commited or Discard. Basing on this, the answers would be:

Should a txn query operate on an older version of the data

It should operate over the data in the datastore + what was written to the transaction so far. It should not have access to data in other uncommitted batches and txns.

should it block write operations,

it shouldn't block write operations. Direct or writes through other txs/batches should be working.

should different writes block or fail on commit

operations accumulated on the txn should either fail or succed altogether.

Interestingly, this batch is already almost transactional, because it only commits when asked. The only thing missing is atomicity. If one of the interim writes fail, all the previous ones are not reverted. Also, the Read side is missing.

[gammazero]

@Wondertan

Can you submit a PR for your proposed implementation of a TxnDatastore. We can then evaluate if it is something specific to your test cases, or if it is more generally useful and should be part of our library.

I think for your intended uses, implementing TxnDatastore for a basic datastore is useful. If it is useful for you, then it is likely useful for others. I think the part that is ambiguous about the TxnDatastore interface is what if any guarantees are made by the Txn interface. The only stated guarantee is that Commit returns an error if the transaction was stale (whatever that means), or not committed to the datastore (for whatever reason).

Such an implementation will need to decide how to detect if committing a transaction should fail, and what rules for consistency to follow. For example:

• if tx1 deletes a key and tx2 updates that key, should tx2 fail or should it write the key with a new value?
• if tx1 increments the values of keys A, B, C, and tx2 reads the values of A, B, C, then transaction should either see all values as before tx1 or all values as after tx1.

Whatever the criteria for Commit succeeding or failing should be clear in the implementation. Seems like all that is being asked for here is that all the changes happen together or they don't. How exactly the datastore chooses to do that (locking, invalidation, both, ...) may not really matter that much.

Idea: A simple form of MVCC where reads operate on a snapshot seen when the transaction was created, and writes create a new version. If a transaction with write ops is committed, and sees that someone else has already updated the version, then fail the transaction. Again, maybe this does not matter so much as there is a way to make calls to Commit fail to ensure tests handle that.