Some groundwork for pipelining the store

[lutter]

This PR contains some fairly simple groundwork in preparation of pipelining the WritableStore. It might be good to merge that now since it touches things in quite a few places outside the store. Pipelining proper should then mostly happen within the store.

The biggest functional change in this PR is that the WritableAgent caches the block pointer and cursor in memory rather than getting it from the database.

[leoyvens]

Two points, which became easier to address after #3085:

1. I think we can make the ownership of the writable agent more precise, by having the instance manager own it directly rather than have the store serve as a registry of writable agents (TIL this is the multiton pattern). It's the only place that calls writable_store, except for the polling block stream which still calls deployment_synced on it. But that's moving to the instance manager after Make Firehose subgraphs have their sync status be updated correctly #3047, and it could be moved to its own trait anyways.

2. The instance manager is the only place where the block cursor and ptr are queried, at block stream construction. We could change this to be queried only at subgraph startup, and have the instance manager keep track of the latest block cursor and ptr it sent to the store (on apply and revert of block operations), rather than querying the writable agent for it. Btw, well noticed that the firehose cursor isn't updated on revert_block_operations, this seems wrong, ping @maoueh.

[maoueh]

Yep indeed, that's wrong. We should update the cursor in this case also definitely.

[lutter]

1. I think we can make the ownership of the writable agent more precise, by having the instance manager own it directly rather than have the store serve as a registry of writable agents (TIL this is the [multiton](https://wiki.c2.com/?MultitonPattern) pattern). It's the only place that calls `writable_store`, except for the polling block stream which still calls `deployment_synced` on it. But that's moving to the instance manager after [Make Firehose subgraphs have their sync status be updated correctly #3047](https://github.com/graphprotocol/graph-node/issues/3047), and it could be moved to its own trait anyways.

The problem with this is that writable stores are also constructed in tests - I had an initial go at that, and they are all over the place when it comes to constructing writable stores. But in the tests, the pattern will need to be

• make some changes to the store
• wait for those changes to actually hit the database
• perform some queries to make sure the db is in the expected state

I suspect there's some utility we can add to the test-store to make that all easier that would mimic the instance manager's handling of writable stores, but it's still something I need to figure out.

2. The instance manager is the only place where the block cursor and ptr are queried, at block stream construction. We could change this to be queried only at subgraph startup, and have the instance manager keep track of the latest block cursor and ptr it sent to the store (on apply and revert of block operations), rather than querying the writable agent for it. Btw, well noticed that the firehose cursor isn't updated on `revert_block_operations`, this seems wrong, ping @maoueh.

I like that - we could change it so that block cursor and ptr are returned from start_subgraph_deployment, and can't be queried through a WritableStore anymore.

[lutter]

Which leaves the question: in terms of this PR, what should change in it? Just remove the commit store: Make SubgraphStore.writable idempotent and change things after this has been committed?

[leoyvens]

@lutter I'd be fine to merge this with that commit removed, but how does that leave the WritableAgent, will it be dead code?

[lutter]

@lutter I'd be fine to merge this with that commit removed, but how does that leave the WritableAgent, will it be dead code?

I just rebased the branch to latest master.

Thinking some more about making SubgraphStore.writable idempotent (a 'multiton'), I don't think there's a safe way to make sure that writable is called at most once for any given deployment. And once we write async to the store, having two WritableStore instances for the same deployment will result in fireworks/incorrect data. IMHO, making writable not idempotent is just too dangerous since it's too easy to violate the constraint "call this only once for any given deployment for the lifetime of the process" in the future, and you'll get no warning that the constraint was violated.

[leoyvens]

Do you think it's actually feasible to error in SubgraphStore::writable if there are outstanding references to the writable agent, by checking the ref count? That would be a strong invariant check.

[lutter]

Do you think it's actually feasible to error in SubgraphStore::writable if there are outstanding references to the writable agent, by checking the ref count? That would be a strong invariant check.

We'd still need a map from deployment id to that agent/writable; at the very least, we'd need to keep a HashSet of deployment ids for which we already made a WritableStore. It will also not work for tests which call writable(..) all over the place; I think it's best to keep maintaining a map to ensure idempotency.

[leoyvens]

@lutter Yes, I was thinking of how we could leverage the map to check an invariant that's truly useful. Having two instances of WritableAgent would be suspicious, but having two references to the same agent would also be suspicious. So the multiton doesn't prevent misuse, it just allows a different misuse. The misuse of two references is actually the scary one to me, because the inconsistencies it can create are more subtle and therefore harder to catch in tests.

But I'll defer to you if you prefer to keep the current approach which deals with WritableAgent through references. I'd perhaps suggest moving the mutex to a wrapping WritableAgent(Arc<Mutex<InnerWritableAgent>>) so that you can write &mut self methods. And then you could consider having a misuse check that errors if the mutex is already held by another thread when trying to acquire it.