core: Store inputs (reducer info + args) in commitlog #1091
Conversation
kim
requested review from
cloutiertyler,
gefjon,
coolreader18 and
joshua-spacetime
crates/core/src/db/relational_db.rs
Outdated
| pub(crate) fn commit_tx_with_reducer_info( | ||
| &self, | ||
| ctx: &ExecutionContext, | ||
| reducer_info: Option<ReducerOp<'_>>, |
There was a problem hiding this comment.
This does not exactly spark joy. How do we feel about repurposing ExecutionContext to optionally contain more metadata about the reducer?
There was a problem hiding this comment.
I'd ask @joshua-spacetime what the original intention for ExecutionContext was first. I know it was for metrics, but did he also have this kind of thing in mind?
There was a problem hiding this comment.
No objections @kim. That's what it's for.
crates/core/src/db/relational_db.rs
Outdated
| // - write to the reducer log | ||
| // (sometimes used to assert that a reducer indeed ran) | ||
| // - schedule a reducer to run at a later time | ||
| // - potentially in the future: perform RPCs |
There was a problem hiding this comment.
Not sure what the first one is but in the near future the second and third ones will write to the log. Writing to the log for RPCs is necessary to ensure exactly once delivery/processing (because in the event of a crash you need to be able to retransmit those messages for which you had not yet received an ack). You can also imagine a world in which we would only want to send RPCs to other databases iff the transactions succeeds.
I think it might also be related to distributed transactions as well if we ever wanted to do that. i.e. transactions between actors.
Basically things change quite a bit if we allow side effects in reducers which we currently do not do (or shouldn't do but do do with scheduled reducers).
Definitely something to noodle on into the future for sure.
There was a problem hiding this comment.
Actually, I meant to change the logic to ignore all transactions which do not mutate the database. But, that is wrong:
A module may not define connect / disconnect, or only define one of them. Or define them, but not modify any tables. Or worse, return an error.
I don't know what to do in the last case, but ignoring that we do indeed need to write out transactions which originate from reducer calls but don't modify the database.
|
Reroll:
|
| // timetravel queries may benefit from seeing all inputs, even if | ||
| // the database state did not change. | ||
| let is_noop = || inserts.is_empty() && deletes.is_empty(); | ||
| let is_connect_disconnect = |ctx: &ReducerContext| { |
There was a problem hiding this comment.
This thought is sort of unrelated to this commit, but I'll just note that we should really make sure these reducers are not callable from outside of SpacetimeDB. I don't actually know that they aren't.
There was a problem hiding this comment.
Good catch ... "dunder" reducers are callable like any other reducer afaics. Because, they are just like any other reducer?
Prerequisite for auto-disconnect after a database crash, requested for analytics purposes.
Prerequisite for auto-disconnect after a database crash, requested for analytics purposes.
Expected complexity level and risk
2
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!