From acdd42c7c8a0beac4635b3ac903ccf482ec2bf2f Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Thu, 22 Mar 2018 17:36:20 +0200 Subject: [PATCH] Merge "Fix abort during counter table read-on-delete" from Tomasz " This fixes an abort in an sstable reader when querying a partition with no clustering ranges (happens on counter table mutation with no live rows) which also doesn't have any static columns. In such case, the sstable_mutation_reader will setup the data_consume_context such that it only covers the static row of the partition, knowing that there is no need to read any clustered rows. See partition.cc::advance_to_upper_bound(). Later when the reader is done with the range for the static row, it will try to skip to the first clustering range (missing in this case). If clustering_ranges_walker tells us to skip to after_all_clustering_rows(), we will hit an assert inside continuous_data_consumer::fast_forward_to() due to attempt to skip past the original data file range. If clustering_ranges_walker returns before_all_clustering_rows() instead, all is fine because we're still at the same data file position. Fixes #3304. " * 'tgrabiec/fix-counter-read-no-static-columns' of github.com:scylladb/seastar-dev: tests: mutation_source_test: Test reads with no clustering ranges and no static columns tests: simple_schema: Allow creating schema with no static column clustering_ranges_walker: Stop after static row in case no clustering ranges (cherry picked from commit 054854839ae5f72ddadaa1a0058a6a91607ac777) --- clustering_ranges_walker.hh | 2 +- tests/mutation_source_test.cc | 55 +++++++++++++++++++++++++++++++++++ tests/simple_schema.hh | 5 ++-- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/clustering_ranges_walker.hh b/clustering_ranges_walker.hh index 3682ccdfc9..c1d710fca0 100644 --- a/clustering_ranges_walker.hh +++ b/clustering_ranges_walker.hh @@ -70,7 +70,7 @@ public: { if (!with_static_row) { if (_current == _end) { - _current_start = _current_end = position_in_partition_view::after_all_clustered_rows(); + _current_start = position_in_partition_view::before_all_clustered_rows(); } else { _current_start = position_in_partition_view::for_range_start(*_current); _current_end = position_in_partition_view::for_range_end(*_current); diff --git a/tests/mutation_source_test.cc b/tests/mutation_source_test.cc index b0f068493b..f44c86aae6 100644 --- a/tests/mutation_source_test.cc +++ b/tests/mutation_source_test.cc @@ -822,6 +822,7 @@ static void test_query_only_static_row(populate_fn populate) { auto pkeys = s.make_pkeys(1); mutation m1(s.schema(), pkeys[0]); + m1.partition().apply(s.new_tombstone()); s.add_static_row(m1, "s1"); s.add_row(m1, s.make_ckey(0), "v1"); s.add_row(m1, s.make_ckey(1), "v2"); @@ -846,6 +847,59 @@ static void test_query_only_static_row(populate_fn populate) { .produces(m1, slice.row_ranges(*s.schema(), m1.key())) .produces_end_of_stream(); } + + // query just a static row, single-partition case + { + auto slice = partition_slice_builder(*s.schema()) + .with_ranges({}) + .build(); + auto prange = dht::partition_range::make_singular(m1.decorated_key()); + assert_that(ms.make_reader(s.schema(), prange, slice)) + .produces(m1, slice.row_ranges(*s.schema(), m1.key())) + .produces_end_of_stream(); + } +} + +static void test_query_no_clustering_ranges_no_static_columns(populate_fn populate) { + simple_schema s(simple_schema::with_static::no); + + auto pkeys = s.make_pkeys(1); + + mutation m1(s.schema(), pkeys[0]); + m1.partition().apply(s.new_tombstone()); + s.add_row(m1, s.make_ckey(0), "v1"); + s.add_row(m1, s.make_ckey(1), "v2"); + + mutation_source ms = populate(s.schema(), {m1}); + + { + auto prange = dht::partition_range::make_ending_with(dht::ring_position(m1.decorated_key())); + assert_that(ms.make_reader(s.schema(), prange, s.schema()->full_slice())) + .produces(m1) + .produces_end_of_stream(); + } + + // multi-partition case + { + auto slice = partition_slice_builder(*s.schema()) + .with_ranges({}) + .build(); + auto prange = dht::partition_range::make_ending_with(dht::ring_position(m1.decorated_key())); + assert_that(ms.make_reader(s.schema(), prange, slice)) + .produces(m1, slice.row_ranges(*s.schema(), m1.key())) + .produces_end_of_stream(); + } + + // single-partition case + { + auto slice = partition_slice_builder(*s.schema()) + .with_ranges({}) + .build(); + auto prange = dht::partition_range::make_singular(m1.decorated_key()); + assert_that(ms.make_reader(s.schema(), prange, slice)) + .produces(m1, slice.row_ranges(*s.schema(), m1.key())) + .produces_end_of_stream(); + } } void test_streamed_mutation_forwarding_succeeds_with_no_data(populate_fn populate) { @@ -967,6 +1021,7 @@ void run_mutation_reader_tests(populate_fn populate) { test_streamed_mutation_forwarding_is_consistent_with_slicing(populate); test_range_queries(populate); test_query_only_static_row(populate); + test_query_no_clustering_ranges_no_static_columns(populate); } void test_next_partition(populate_fn populate) { diff --git a/tests/simple_schema.hh b/tests/simple_schema.hh index 5d10617a8c..d609cdb007 100644 --- a/tests/simple_schema.hh +++ b/tests/simple_schema.hh @@ -47,11 +47,12 @@ public: return {new_timestamp(), gc_clock::now()}; } public: - simple_schema() + using with_static = bool_class; + simple_schema(with_static ws = with_static::yes) : _s(schema_builder("ks", "cf") .with_column("pk", utf8_type, column_kind::partition_key) .with_column("ck", utf8_type, column_kind::clustering_key) - .with_column("s1", utf8_type, column_kind::static_column) + .with_column("s1", utf8_type, ws ? column_kind::static_column : column_kind::regular_column) .with_column("v", utf8_type) .build()) , _v_def(*_s->get_column_definition(to_bytes("v")))