Conversation
|
This change is |
I can't prove that this lock provides any necessary synchronization. It was introduced in the days before prop-eval KV and finer-grained dependency tracking in the command queue. I'm not actually suggesting that we land this PR in the 2.1 timeframe, but I wanted to get a CI run and a conversation started. Release note: None
|
The more I look into this, the more confidence I gain in this change. Anywhere we were blocking reads with this mutex, we needed to block writes, too—and we invented separate mechanisms for doing so as we played whack-a-mole with the resulting serializability violations. See #17109 for one such example. More details in #core. |
nvb
left a comment
There was a problem hiding this comment.
This change LGTM but I don't know enough about this BlockReads option to know whether it's protecting something subtle or what we need to do to prove that it's not (no longer?) necessary.
bdarnell
left a comment
There was a problem hiding this comment.
I don't think the propEvalKV command queue changes made this mutex obsolete - it's not about KV-level read/write conflicts, it's about the timestamp cache (which is mutated by reads). However, I think the combination of #17109 (for leases) and the move to a store-level timestamp cache (as opposed to replica-level, which means splits no longer copy the timestamp cache or otherwise have special interactions with it) may have done the trick.
Reviewed 9 of 9 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
Yes, I agree. I consider #17109 to be part of the move to prop-eval KV since things prop-eval KV was incorrect without it. :) I'll update the commit message to be more precise before I merge. I'm not going to merge for a while yet since I won't be able to own the fallout, if any, until range merges are little more stable. If someone else would like to take responsibility for this change, though, feel free to commandeer this PR and merge. |
tbg
left a comment
There was a problem hiding this comment.
I agree with Ben's comment. Please add an explanation of the scenarios in an appropriate place, for example the split trigger. (I know you're just letting this sit for now, so no rush).
Reviewable status:
|
There's some discussion in #32583 that questions the safety of this change. |
|
Ok, sounds like this isn't safe after all! |
|
Discussed this with tobias today -- a solution would be to have the span latch manager take over the concern using a fake mvcc request at timestamp 0. Will need to investigate further. |
I can't prove that this lock provides any necessary synchronization. It
was introduced in the days before prop-eval KV and finer-grained
dependency tracking in the command queue.
I'm not actually suggesting that we land this PR in the 2.1 timeframe,
but I wanted to get a CI run and a conversation started.
Release note: None