From 778f5adde70cda023c558dc7ad7f1a8bc8c6e962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Tue, 26 Oct 2021 10:56:43 +0300 Subject: [PATCH 1/5] mutation_partition: row: make row marker shadowing symmetric Currently row marker shadowing the shadowable tombstone is only checked in `apply(row_marker)`. This means that shadowing will only be checked if the shadowable tombstone and row marker are set in the correct order. This at the very least can cause flakyness in tests when a mutation produced just the right way has a shadowable tombstone that can be eliminated when the mutation is reconstructed in a different way, leading to artificial differences when comparing those mutations. This patch fixes this by checking shadowing in `apply(shadowable_tombstone)` too, making the shadowing check symmetric. There is still one vulnerability left: `row_marker& row_marker()`, which allow overwriting the marker without triggering the corresponding checks. We cannot remove this overload as it is used by compaction so we just add a comment to it warning that `maybe_shadow()` has to be manually invoked if it is used to mutate the marker (compaction takes care of that). A caller which didn't do the manual check is mutation_source_test: this patch updates it to use `apply(row_marker)` instead. Fixes: #9483 Tests: unit(dev) Closes #9519 --- mutation_partition.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/mutation_partition.hh b/mutation_partition.hh index e9db79c9ed..7194bfe534 100644 --- a/mutation_partition.hh +++ b/mutation_partition.hh @@ -830,6 +830,7 @@ public: void apply(shadowable_tombstone deleted_at) { _deleted_at.apply(deleted_at, _marker); + maybe_shadow(); } void apply(row_tombstone deleted_at) { From 9c66c9b3f004ad54262d01de25e1169d5b3f6330 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Aug 2022 16:52:48 +0200 Subject: [PATCH 2/5] db: mutation_partition: Maintain shadowable tombstone invariant when applying a hard tombstone When the row has a live row marker which shadows the shadowable tombstone, the shadowable tombstone should not be effective. The code assumes that _shadowable always reflects the current tombstone, so maybe_shadow() needs to be called whenever marker or regular tombstone changes. This was not ensured by row::apply(tombstone). This causes problems in tests which use random_mutation_generator, which generates mutations which would violate this invariant, and as a result, mutation commutativity would be violated. I am not aware of problems in production code. --- mutation_partition.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/mutation_partition.hh b/mutation_partition.hh index 7194bfe534..8c754dc74f 100644 --- a/mutation_partition.hh +++ b/mutation_partition.hh @@ -826,6 +826,7 @@ public: void apply(tombstone deleted_at) { _deleted_at.apply(deleted_at); + maybe_shadow(); } void apply(shadowable_tombstone deleted_at) { From 56e5b6f095ccb953d31ad75fe120043c2e8a9814 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Aug 2022 16:55:55 +0200 Subject: [PATCH 3/5] db: mutation_partition: Drop unnecessary maybe_shadow() It is performed inside row_tombstone::apply() invoked in the preceding line. --- mutation_partition.hh | 1 - 1 file changed, 1 deletion(-) diff --git a/mutation_partition.hh b/mutation_partition.hh index 8c754dc74f..5b12345ba7 100644 --- a/mutation_partition.hh +++ b/mutation_partition.hh @@ -831,7 +831,6 @@ public: void apply(shadowable_tombstone deleted_at) { _deleted_at.apply(deleted_at, _marker); - maybe_shadow(); } void apply(row_tombstone deleted_at) { From 3d9efee3bfedfc6e39dde98a9c5ac546e047f5ba Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Aug 2022 16:57:09 +0200 Subject: [PATCH 4/5] test: random_mutation_generator: Workaround for non-associativity of mutations with shadowable tombstones Given 3 row mutations: m1 = { marker: {row_marker: dead timestamp=-9223372036854775803}, tombstone: {row_tombstone: {shadowable tombstone: timestamp=-9223372036854775807, deletion_time=0}, {tombstone: none}} } m2 = { marker: {row_marker: timestamp=-9223372036854775805} } m3 = { tombstone: {row_tombstone: {shadowable tombstone: timestamp=-9223372036854775806, deletion_time=2}, {tombstone: none}} } We get different shadowable tombstones depending on the order of merging: (m1 + m2) + m3 = { marker: {row_marker: dead timestamp=-9223372036854775803}, tombstone: {row_tombstone: {shadowable tombstone: timestamp=-9223372036854775806, deletion_time=2}, {tombstone: none}} m1 + (m2 + m3) = { marker: {row_marker: dead timestamp=-9223372036854775803}, tombstone: {row_tombstone: {shadowable tombstone: timestamp=-9223372036854775807, deletion_time=0}, {tombstone: none}} } The reason is that in the second case the shadowable tombstone in m3 is shadwed by the row marker in m2. In the first case, the marker in m2 is cancelled by the dead marker in m1, so shadowable tombstone in m3 is not cancelled (the marker in m1 does not cancel because it's dead). This wouldn't happen if the dead marker in m1 was accompanied by a hard tombstone of the same timestamp, which would effectively make the difference in shadowable tombstones irrelevant. Found by row_cache_test.cc::test_concurrent_reads_and_eviction. I'm not sure if this situation can be reached in practice (dead marker in mv table but no row tombstone). Work it around for tests by producing a row tombstone if there is a dead marker. Refs #11307 --- test/lib/mutation_source_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/lib/mutation_source_test.cc b/test/lib/mutation_source_test.cc index 1b5c4d6e3e..bc6b9bc9eb 100644 --- a/test/lib/mutation_source_test.cc +++ b/test/lib/mutation_source_test.cc @@ -2247,6 +2247,11 @@ public: if (_not_dummy_dist(_gen)) { deletable_row& row = m.partition().clustered_row(*_schema, ckey, is_dummy::no, continuous); row.apply(random_row_marker()); + if (!row.marker().is_missing() && !row.marker().is_live()) { + // Mutations are not associative if dead marker is not matched with a dead row + // due to shadowable tombstone merging rules. See #11307. + row.apply(tombstone(row.marker().timestamp(), row.marker().deletion_time())); + } if (_bool_dist(_gen)) { set_random_cells(row.cells(), column_kind::regular_column); } else { From 5a9df433c6b587fe09f535a958509e0c32f3cb11 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Wed, 17 Aug 2022 17:18:57 +0200 Subject: [PATCH 5/5] test: mutation_test: Add explicit test for mutation commutativity --- test/boost/mutation_test.cc | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/boost/mutation_test.cc b/test/boost/mutation_test.cc index e7b47b8762..30ee4e2112 100644 --- a/test/boost/mutation_test.cc +++ b/test/boost/mutation_test.cc @@ -1852,6 +1852,29 @@ SEASTAR_TEST_CASE(test_continuity_merging_of_complete_mutations) { return make_ready_future<>(); } +SEASTAR_TEST_CASE(test_commutativity_and_associativity) { + random_mutation_generator gen(random_mutation_generator::generate_counters::no); + gen.set_key_cardinality(7); + + for (int i = 0; i < 10; ++i) { + mutation m1 = gen(); + m1.partition().make_fully_continuous(); + mutation m2 = gen(); + m2.partition().make_fully_continuous(); + mutation m3 = gen(); + m3.partition().make_fully_continuous(); + + assert_that(m1 + m2 + m3) + .is_equal_to(m1 + m3 + m2) + .is_equal_to(m2 + m1 + m3) + .is_equal_to(m2 + m3 + m1) + .is_equal_to(m3 + m1 + m2) + .is_equal_to(m3 + m2 + m1); + } + + return make_ready_future<>(); +} + SEASTAR_TEST_CASE(test_continuity_merging) { return seastar::async([] { simple_schema table;