Transactional sled pinstore #442

koivunej · 2021-01-07T13:58:00Z

This enables the pinstore to use transactions for almost all operations. Big ticket TODOs remaining:

handling duplicate indirect pins (missing test)
handling pin combinations (missing test)
cidv0 compatible cidv1 normalization (missing test)
the str representation could probably be changed?
sled usage still needs to be wrapped in spawn_blocking

In smaller changes:

makes PinMode Copy
extracts a PinModeRequirement out of the Option<PinMode> which is hopefully more readable (list, query)

CC: @fetchadd, got the PR I promised made. Do you have any ideas? Sadly the changeset is probably quite difficult to review. I could split it up to have just the small fixes followed by the transactional stuff, but the smaller changes can probably by diffing only up until some commits.

this wasn't done originally even if PinMode was quite simple, just to see if there needs to be more complication later on.

perhaps the launder is more used term here.

fetchadd

Great job, transaction is better than Batch, as it makes operations like read then write atomic, the batch can only guarantee the a squence of write things to be atomic 👍 .

.. I find a pontential risk that a transaction can block another transaction

fetchadd · 2021-01-08T03:33:59Z

src/repo/kv.rs

 #[async_trait]
 impl PinStore for KvDataStore {
-    async fn is_pinned(&self, block: &Cid) -> Result<bool, Error> {
-        is_pinned(self, block)
+    async fn is_pinned(&self, cid: &Cid) -> Result<bool, Error> {


Maybe this is the only place where transaction is not needed, as sled's single-key operations are atomic :)

however, this looking like it can be just an atomic read makes me think it must be the wrong abstraction.. but if I recall correctly, this is called when removing a block through /api/v0/block/rm so ... I guess it needs to stay for now.

fetchadd · 2021-01-08T04:02:03Z

src/repo/kv.rs

+                match already_pinned {
+                    Some(PinMode::Recursive) => return Ok(false),
+                    Some(mode @ PinMode::Direct) | Some(mode @ PinMode::Indirect) => {
+                        // FIXME: this is probably another lapse in tests that both direct and


I have tried it in go-ipfs, yes, when a cid is pinned as indirect pin, after another recursive pin, the cid will be marked as recursive pin. For existed direct pin, I cannot get a cid marked as direct, but in "go-ipfs-pinner/dspinner/pin.go", there is

Whoops, forgot to reply to this one. Yeah, this is a good point.

I've implemented (or at least tried to) the mem and fs so that the pins would not replace other pins, and I am surprised you found this in go-ipfs. I might also misremember, I did go back and forth here a lot. But I guess I'll need to confirm my thinking in upcoming PRs with more test cases, following the addition of spawn_blocking.

fetchadd · 2021-01-08T04:54:08Z

src/repo/kv.rs

-
-                    _ if cid_str_with_prefix.starts_with("i") => {
-                        PinMode::Indirect
+        // FIXME: this is still blocking ... might not be a way without a channel


If the pin listing is mainly used for GC, there will not be many concurrent call sto it1 , so may be a new thread can be spawned by something like threadpool::spawn to make the listing non-blocking.

I wonder why the PinStore and DataStore should be coupled, if it must be, can pin data be seperately putted in a alone sled tree. Then the filter of key that it should start with "pin." can be removed.

mainly used for GC, there will not be many concurrent call to it

Since it's available from the http api, I'd consider it beign callable at all times. For GC... I think the transactions might be helping out with it a lot. However, I am not too sure if we should attempt to build a similar GC than what go-ipfs has (not sure what is the present implementation) but I was thinking of trying to go through more of the route of maintaining a collectable set at all times through reference counting.

The reference counting would of probably require serializing all of the documents as graphs inside the blockstore, paving a way for a more monolithic store instead of trying to go with a number of different stores which are pluggable. I can't really see an design of such composed store which would be anywhere near consistent or durable.

so maybe a new thread can be spawned by something like threadpool::spawn

tokio already has spawn_blocking so I'd rather not entangle a new threadpool implementation here. I did not yet use the spawn_blocking as that'd mess up the indentation at least and make this PR even less reviewable. I'll do this in a follow-up.

I wonder why the PinStore and DataStore should be coupled

There is no particular reason, this was more of a "convenient" quick decision I made to enable hacking the memory and fs based stores together, getting the "ball rolling" so to speak. However, I am still thinking that perhaps we should just put everything of Repo behind a single monolithic sled/database implementation and not even try to view these as separate. Perhaps things like non-local storage of blocks (for example in a s3) should be viewed as extensions to a monolithic database backend rather than "interchangeable blocks". Abstracting over databases is really difficult either way.

pin data be separatedly put in a standalone sled tree. Then the filter of key that is should start with "pin." can be removed.

My thoughts exactly! Tried to keep a lid on the number of changes which already spiraled out of control so kept the current way :) I tried to write some more ideas in TODOs of the get_pin_key(cid: &Cid, mode: &Mode) -> String fn.

fetchadd · 2021-01-08T04:56:22Z

src/repo/kv.rs

+
+            // as we might loop over an over on the tx we might need this
+            for id in ids.iter() {
+                // FIXME: this is blocking ...


The same, considering no may calls at the same time, a new thread may be used to spawn the call.

RE: blocking in general. Doing a lot of blocking stuff will be detrimental to any runtimes (such as tokio) performance as futures simply should not block, and we cannot rely on the database being cached at any times (re: sled docs on the topic). So will be following-up with a spawn_blocking linked in the above comment.

koivunej · 2021-01-08T08:47:15Z

.. I find a pontential risk that a transaction can block another transaction

My understanding is that sled will take care of this given the transaction scripts are Fn which will be just reinvoked if they return a conflict. I am thinking they should be FnMut but didn't yet look how exactly are they called inside sled; FnMut would make it clear that the query can for example only allocate the vec once, and keep trying to fill it over and over again.

fetchadd · 2021-01-08T10:03:51Z

.. I find a pontential risk that a transaction can block another transaction

My understanding is that sled will take care of this given the transaction scripts are Fn which will be just reinvoked if they return a conflict. I am thinking they should be FnMut but didn't yet look how exactly are they called inside sled; FnMut would make it clear that the query can for example only allocate the vec once, and keep trying to fill it over and over again.

There is a example(sorry, this cannot be runned on the playground as there is a sled external crate), when a transaction is not over, anther transaction will be blocked. So like query method of PinStore, if the transaction takes up a long time, all other requests using the same sled db will be blocked, so the transaction scope should be as smaller as possible. I have no idea how to slove this, maybe the blocking is reasonable that the db must be locked for the atomic operations.

koivunej · 2021-01-08T10:48:37Z

Thanks for the example. Yeah, using sleeps or external waits inside would of course make it worse, similarly to blocking inside an async runtime. I had been expecting that sled would support multiple writers but perhaps not for transactions...? I should look at it more deeply.

I have no idea how to slove this, maybe the blocking is reasonable that the db must be locked for the atomic operations.

Yeah I am guessing it's just one of those things which doesn't need solving. However it'd be a good idea to have some sort of mechanism to catch the slowing down, though running these in a spawn_blocking will already apply some queuing to it which might be enough to sort of side-step this issue. I hear the 2021 will be the year of observability so hopefully we'll get some advances on that front :)

RE: the PR I assume this would be 👍 from you?

fetchadd

Good jobs! 👍 Yes, 2021 will be the year of observability hope

koivunej · 2021-01-08T12:46:42Z

great, thanks!

bors r+

bors · 2021-01-08T13:03:38Z

Build succeeded:

ci

koivunej added 17 commits January 7, 2021 15:46

chore: mention sled pr in changelog

c69c09d

refactor: rename used methods without underscore

b3c439e

refactor: prefer into_owned

e1685c3

refactor: simplify matches

e3de305

refactor: make PinMode copy

8f1b1a2

this wasn't done originally even if PinMode was quite simple, just to see if there needs to be more complication later on.

refactor: extract common type for the pinmode requirement

6e54cc1

refactor: remove cid cloning by taking ownership

51675f5

doc: note bunch of fixmes

65fddd4

refactor: rewrite list without async_stream

ac21130

refactor: remove extra types, create with max capacity

b30e12a

feat: transactions in the sled pinstore

9db1d86

fix: inserting initial direct pin

6cc246c

refactor: add messages to common_tests asserts

e1cd4c4

feat: flush after modifying pinstore

9b6f065

refactor: use less full names

c51d37c

chore: rename unwrap_tx_res => launder

8871b3d

perhaps the launder is more used term here.

chore: clippy warnings

edd0ac9

koivunej force-pushed the sled_pinstore_changes branch from 5d7c382 to edd0ac9 Compare January 7, 2021 14:07

chore: record this pr in the changelog as well

c060dc4

fetchadd reviewed Jan 8, 2021

View reviewed changes

koivunej marked this pull request as ready for review January 8, 2021 10:48

fetchadd approved these changes Jan 8, 2021

View reviewed changes

bors bot merged commit 5d9cae5 into rs-ipfs:master Jan 8, 2021

koivunej deleted the sled_pinstore_changes branch January 12, 2021 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transactional sled pinstore #442

Transactional sled pinstore #442

koivunej commented Jan 7, 2021 •

edited

Loading

fetchadd left a comment •

edited

Loading

fetchadd Jan 8, 2021

koivunej Jan 8, 2021

fetchadd Jan 8, 2021

koivunej Jan 8, 2021

fetchadd Jan 8, 2021 •

edited

Loading

koivunej Jan 8, 2021

fetchadd Jan 8, 2021

koivunej Jan 8, 2021

koivunej commented Jan 8, 2021

fetchadd commented Jan 8, 2021 •

edited

Loading

koivunej commented Jan 8, 2021

fetchadd left a comment

koivunej commented Jan 8, 2021

bors bot commented Jan 8, 2021

Transactional sled pinstore #442

Transactional sled pinstore #442

Conversation

koivunej commented Jan 7, 2021 • edited Loading

fetchadd left a comment • edited Loading

Choose a reason for hiding this comment

fetchadd Jan 8, 2021

Choose a reason for hiding this comment

koivunej Jan 8, 2021

Choose a reason for hiding this comment

fetchadd Jan 8, 2021

Choose a reason for hiding this comment

koivunej Jan 8, 2021

Choose a reason for hiding this comment

fetchadd Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

koivunej Jan 8, 2021

Choose a reason for hiding this comment

fetchadd Jan 8, 2021

Choose a reason for hiding this comment

koivunej Jan 8, 2021

Choose a reason for hiding this comment

koivunej commented Jan 8, 2021

fetchadd commented Jan 8, 2021 • edited Loading

koivunej commented Jan 8, 2021

fetchadd left a comment

Choose a reason for hiding this comment

koivunej commented Jan 8, 2021

bors bot commented Jan 8, 2021

koivunej commented Jan 7, 2021 •

edited

Loading

fetchadd left a comment •

edited

Loading

fetchadd Jan 8, 2021 •

edited

Loading

fetchadd commented Jan 8, 2021 •

edited

Loading