From 5779230d287d40a12729ef4c8ca1730abc04ba15 Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Mon, 10 Jul 2023 22:55:08 +0200 Subject: [PATCH] group0_state_machine: use correct comparison for timeuuids in `merger` In d2a4079bbeef21d051d07e317d871ad54efd1896, `merger` was modified so that when we merge a command, `last_group0_state_id` is taken to be the maximum of the merged command's state_id and the current `last_group0_state_id`. This is necessary for achieving the same behavior as if the commands were applied individually instead of being merged -- where we take the maximum state ID from `group0_history` table which was applied until now (because the table is sorted using the state IDs and we take the greatest row). However, a subtle bug was introduced -- the `std::max` function uses the `utils::UUID` standard comparison operator which is unfortunately not the same as timeuuid comparison that Scylla performs when sorting the `group0_history` table. So in rare cases it could return the *smaller* of the two timeuuids w.r.t. the correct timeuuid ordering. This would then lead to commands being applied which should have been turned to no-ops due to the `prev_state_id` check -- and then, for example, permanent schema desync or worse. Fix it by using the correct comparison method. Fixes: #14600 --- service/raft/group0_state_machine.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/service/raft/group0_state_machine.cc b/service/raft/group0_state_machine.cc index 18d2b88b92..fb4125eceb 100644 --- a/service/raft/group0_state_machine.cc +++ b/service/raft/group0_state_machine.cc @@ -121,7 +121,12 @@ future<> group0_state_machine::apply(std::vector command) { void add(group0_command&& cmd, size_t added_size) { slogger.trace("add to merging set new_state_id: {}", cmd.new_state_id); auto m = convert_history_mutation(std::move(cmd.history_append), sm._sp.data_dictionary()); - last_group0_state_id = std::max(last_group0_state_id, cmd.new_state_id); + // Set `last_group0_state_id` to the maximum of the current value and `cmd.new_state_id`, + // but make sure we compare them the same way timeuuids are compared in clustering keys + // (i.e. in the same order that the history table is sorted). + if (utils::timeuuid_tri_compare(last_group0_state_id, cmd.new_state_id) < 0) { + last_group0_state_id = cmd.new_state_id; + } cmd_to_merge.push_back(std::move(cmd)); size += added_size; if (merged_history_mutation) {