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.