persist-txn: support "eager" physical upper advancement on data shards #23552
Conversation
danhhz
force-pushed
the
persist_txn_eager
branch
3 times, most recently
from
0d7c72b to
33edace
Compare
|
There's a couple WIPs left to polish out, but this is ready for a look! I'll likely pull out the bits to turn it on in CI before merging |
|
MitigationsCompleting required mitigations increases Resilience Coverage.
Bug Hotspots:
|
| // TODO: Ideally the upper advancements would be done concurrently with | ||
| // this apply_le. | ||
| let tidy = self.apply_le(ts).await; | ||
|
|
There was a problem hiding this comment.
Does anything bad happen if we crash here, after the apply_le but before the advancing the physical uppers? One potentially bad scenario:
- Upper of some shard
sis at5. - Apply txns at
10. - Crash.
- Reboot.
- Try and read
sat10.
If at some point during startup we call apply_eager_le(ts') s.t. ts' >= 10, then I think we're fine. Otherwise we might need to wait until the next commit to read the shard?
There was a problem hiding this comment.
Good point, seems like we just need call apply_eager_le in init_ts?
There was a problem hiding this comment.
That sounds like it would work.
There was a problem hiding this comment.
Now that I think about it though, we definitely do at least one table write during bootstrap so it's probably fine as is. Might be better to be explicit about it though.
There was a problem hiding this comment.
Yeah, I realized the same thing and ditto for create_collection, which would also advance them. Added it anyway :), but had to do it in register instead of init_txns.
| // TODO: Ideally the upper advancements would be done concurrently with | ||
| // this apply_le. | ||
| let tidy = self.apply_le(ts).await; | ||
|
|
There was a problem hiding this comment.
Yeah, I realized the same thing and ditto for create_collection, which would also advance them. Added it anyway :), but had to do it in register instead of init_txns.
src/storage-controller/src/lib.rs
Outdated
| tx.clone(), | ||
| txns, | ||
| lazy_persist_txn_tables, | ||
| ); | ||
| let txns_read = TxnsRead::start::<TxnsCodecRow>(txns_client.clone(), txns_id); |
There was a problem hiding this comment.
Realized last night that having this txns_read around is wasteful for eager uppers (plus having it running increases our exposure to bugs slightly), so reworked a touch here to avoid that.
danhhz
force-pushed
the
persist_txn_eager
branch
from
8d1111f to
6a1ef27
Compare
When a txn commits, the "logical upper" of each data shard is advanced but the "physical upper" is only advanced for the data shards involved in the txn. This creates a "lazy upper" situation that is addressed on the read side. This commit introduces the ability to instead advance the upper of all registered data shards, which means we can use all totally normal unchanged read paths. The eager uppers of course don't get the efficiency and cost benefits of lazy uppers, but they'll allow us to derisk the rollout to prod by breaking it into two pieces.
danhhz
force-pushed
the
persist_txn_eager
branch
from
6a1ef27 to
afbdcee
Compare
|
TFTR! |
persist-txn: support "eager" physical upper advancement on data shards
When a txn commits, the "logical upper" of each data shard is advanced
but the "physical upper" is only advanced for the data shards involved
in the txn. This creates a "lazy upper" situation that is addressed on
the read side.
This commit introduces the ability to instead advance the upper of all
registered data shards, which means we can use all totally normal
unchanged read paths. The eager uppers of course don't get the
efficiency and cost benefits of lazy uppers, but they'll allow us to
de-risk the rollout to prod by breaking it into two pieces.
Touches https://github.com/MaterializeInc/database-issues/issues/6685
Touches https://github.com/MaterializeInc/database-issues/issues/6888
Motivation
Tips for reviewer
Ignore the first commit, it's #23588.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.