cache_flat_mutation_reader: use the correct schema in prepare_hash #14305
Conversation
mutation/mutation_cleaner.hh
Outdated
| @@ -24,6 +25,7 @@ class mutation_cleaner_impl final { | |||
| snapshot_list snapshots; | |||
| logalloc::allocating_section alloc_section; | |||
| bool done = false; // true means the worker was abandoned and cannot access the mutation_cleaner_impl instance. | |||
| rwlock merging_enabled; // Allows for pausing the background merging. Used only for testing purposes. | |||
There was a problem hiding this comment.
Is a rwlock really needed? Will you have multiple "reader"s?
In fact I'm not sure a lock is needed at all, since the non-test size only uses trylock. Isn't it enough to have a bool?
There was a problem hiding this comment.
In fact I'm not sure a lock is needed at all, since the non-test size only uses trylock. Isn't it enough to have a bool?
On the non-test size, the worker blocks on the lock.
It's possible to implement this with a bool too. But then I would have to make the returned guard be a separate type, which signals the condition variable when it's destroyed. I figured rwlock was cleaner.
Is a rwlock really needed? Will you have multiple "reader"s?
No, probably not. In fact, I first used a mutex for this. But then I decided that a reentrant/nestable API for pausing would be nicer.
There was a problem hiding this comment.
Changed to an int (like a bool, but reentrant) in v2.
mutation/mutation_partition.cc
Outdated
| } | ||
| merge_some(); | ||
| return stop_iteration::no; | ||
| return with_lock(w->merging_enabled.for_write(), [this] () noexcept { |
There was a problem hiding this comment.
This adds a deferring point after w->done is checked. We must not attempt merge_some() when done is true because region() may no longer be valid after ~mutation_cleaner_impl().
There was a problem hiding this comment.
True. The w->done check has to be inside the lock for this to be correct.
That's what I get for playing with locks...
There was a problem hiding this comment.
I'm deeply ashamed of having authored a concurrency bug. Thank you for saving me from the disgrace of getting it into master.
I hope v2 is correct.
In unit tests, we would want to delay the merging of some MVCC versions to test the transient scenarios with multiple versions present. In many cases this can be done by holding snapshots to all versions. But sometimes (i.e. during schema upgrades) versions are added and scheduled for merge immediately, without a window for the test to grab a snapshot to the new version. This patch adds a pause() method to mutation_cleaner, which ensures that no asynchronous/implicit MVCC version merges happen within the scope of the call. This functionality will be used by a test added in an upcoming patch.
Since `mvcc: make schema upgrades gentle` (51e3b93), rows pointed to by the cursor can have different (older) schema than the schema of the cursor's snapshot. However, one place in the code wasn't updated accordingly, causing a row to be processed with the wrong schema in the right circumstances. This passed through unit testing because it requires a digest-computing cache read after a schema change, and no test exercised this. Fixes scylladb#14110
|
v2:
|
|
CI state |
Since
mvcc: make schema upgrades gentle(51e3b93),rows pointed to by the cursor can have different (older) schema
than the schema of the cursor's snapshot.
However, one place in the code wasn't updated accordingly,
causing a row to be processed with the wrong schema in the right
circumstances.
This passed through unit testing because it requires
a digest-computing cache read after a schema change,
and no test exercised this.
This series fixes the bug and adds a unit test which reproduces the issue.
Fixes #14110