This completes the batch_ and modification_statement rework.
Also touch the private batch_statement::read_command while at it.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Otherwise, it may trip an assertion when the nuderlying
file is closed, as seen in e.g.:
https://jenkins.scylladb.com/view/master/job/scylla-master/job/next/4318/artifact/testlog/x86_64_release/sstable_3_x_test.test_read_rows_only_index.4174.log
```
test/boost/sstable_3_x_test.cc(0): Entering test case "test_read_rows_only_index"
sstable_3_x_test: ./seastar/src/core/fstream.cc:205: virtual seastar::file_data_source_impl::~file_data_source_impl(): Assertion `_reads_in_progress == 0' failed.
Aborting on shard 0.
Backtrace:
0x22557e8
0x2286842
0x7f2799e99a1f
/lib64/libc.so.6+0x3d2a1
/lib64/libc.so.6+0x268a3
/lib64/libc.so.6+0x26788
/lib64/libc.so.6+0x35a15
0x222c53d
0x222c548
0xb929cc
0xc0b23b
0xa84bbf
0x24d0111
```
Decoded:
```
__GI___assert_fail at :?
~file_data_source_impl at ./build/release/seastar/./seastar/src/core/fstream.cc:205
~file_data_source_impl at ./build/release/seastar/./seastar/src/core/fstream.cc:202
std::default_delete<seastar::data_source_impl>::operator()(seastar::data_source_impl*) const at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:85
(inlined by) ~unique_ptr at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:361
(inlined by) ~data_source at ././seastar/include/seastar/core/iostream.hh:55
(inlined by) ~input_stream at ././seastar/include/seastar/core/iostream.hh:254
(inlined by) ~continuous_data_consumer at ././sstables/consumer.hh:484
(inlined by) ~index_consume_entry_context at ././sstables/index_reader.hh:116
(inlined by) std::default_delete<sstables::index_consume_entry_context<sstables::index_consumer> >::operator()(sstables::index_consume_entry_context<sstables::index_consumer>*) const at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:85
(inlined by) ~unique_ptr at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:361
(inlined by) ~index_bound at ././sstables/index_reader.hh:395
(inlined by) ~index_reader at ././sstables/index_reader.hh:435
std::default_delete<sstables::index_reader>::operator()(sstables::index_reader*) const at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:85
(inlined by) ~unique_ptr at /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/unique_ptr.h:361
(inlined by) ~index_reader_assertions at ././test/lib/index_reader_assertions.hh:31
(inlined by) operator() at ./test/boost/sstable_3_x_test.cc:4630
```
Test: unit(dev), sstable_3_x_test.test_read_rows_only_index(release X 10000)
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20211222132858.2155227-1-bhalevy@scylladb.com>
"
The second patch in this series is a mechanical conversion of
reader_concurrency_semaphore to flat_mutation_reader_v2, and caller
updates.
The first patch is needed to pass the test suite, since without it a
real reader version conversion would happen on every entry to and exit
from reader_concurrency_semaphore, which is stressful (for example:
mutation_reader_test.test_multishard_streaming_reader reaches 8191
conversions for a couple of readers, which somehow causes it to catch
SIGSEGV in diverse and seemingly-random places).
Note that in a real workload it is unreasonable to expect readers being
parked in a reader_concurrency_semaphore to be pristine, so
short-circuiting their version conversions will be impossible and this
workaround will not really help.
"
* tag 'rcs-v2-v4' of https://github.com/cmm/scylla:
reader_concurrency_semaphore: convert to flat_mutation_reader_v2
short-circuit flat mutation reader upgrades and downgrades
Make sure that major will compact data in all sstables and memtable,
as tombstones sitting in memtable could shadow data in sstables.
For example, a tombstone in memtable deleting a large partition could
be missed in major, so space wouldn't be saved as expected.
Additionally, write amplification is reduced as data in memtable
won't have to travel through tiers once flushed.
Fixes#9514.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20211217160055.96693-2-raphaelsc@scylladb.com>
"
TWCS perform STCS on a window as long as it's the most recent one.
From there on, TWCS will compact all files in the past window into
a single file. With some moderate write load, it could happen that
there's still some compaction activity in that past window, meaning
that per-window major may miss some files being currently compacted.
As a result, a past window may contain more than 1 file after all
compaction activity is done on its behalf, which may increase read
amplification. To avoid that, TWCS will now make sure that per-window
major is serialized, to make sure no files are missed.
Fixes#9553.
tests: unit(dev).
"
* 'fix_twcs_per_window_major_v3' of https://github.com/raphaelsc/scylla:
TWCS: Make sure major on past window is done on all its sstables
TWCS: remove needless param for STCS options
TWCS: kill unused param in newest_bucket()
compaction: Implement strategy control and wire it
compaction: Add interface to control strategy behavior.
"
Users are adjusted by sprinkling `upgrade_to_v2()` and
`downgrade_to_v1()` where necessary (or removing any of these where
possible). No attempt was made to optimize and reduce the amount of
v1<->v2 conversions. This is left for follow-up patches to keep this set
small.
The combined reader is composed of 3 layers:
1. fragment producer - pop fragments from readers, return them in batches
(each fragment in a batch having the same type and pos).
2. fragment merger - merge fragment batches into single fragments
3. reader implementation glue-code
Converting layers (1) and (3) was mostly mechanical. The logic of
merging range tombstone changes is implemented at layer (2), so the two
different producer (layer 1) implementations we have share this logic.
Tests: unit(dev)
"
* 'combined-reader-v2/v4' of https://github.com/denesb/scylla:
test/boost/mutation_reader_test: add test_combined_reader_range_tombstone_change_merging
mutation_reader: convert make_clustering_combined_reader() to v2
mutation_reader: convert position_reader_queue to v2
mutation_reader: convert make_combined_reader() overloads to v2
mutation_reader: combined_reader: convert reader_selector to v2
mutation_reader: convert combined reader to v2
mutation_reader: combined_reader: attach stream_id to mutation_fragments
flat_mutation_reader_v2: add v2 version of empty reader
test/boost/mutation_reader_test: clustering_combined_reader_mutation_source_test: fix end bound calculation
Allow stopping compaction by type on a given keyspace and list of tables.
Also add api unit test suite that tests the existing `stop_compaction` api
and the new `stop_keyspace_compaction` api.
Fixes#9700Closes#9746
* github.com:scylladb/scylla:
api: storage_service: validate_keyspace: improve exception error message
api: compaction_manager: add stop_keyspace_compaction
api: storage_service: expose validate_keyspace and parse_tables
api: compaction_manager: stop_compaction: fix type description
compaction_manager: stop_compaction: expose optional table*
test: api: add basic compaction_manager test
Replace get_local_storage_proxy() and get_local_storage_proxy() with
constructor-provided references. Some unneeded cases were removed.
Test: unit (dev)
Closes#9816
* github.com:scylladb/scylla:
migration_manager: replace uses of get_storage_proxy and get_local_storage_proxy with constructor-provided reference
migration_manager: don't keep storage_proxy alive during schema_check verb
mm: don't capture storage proxy shared_ptr during background schema merge
mm: remove stats on schema version get
Currently the test assumes that fragments represent weakly monotonic
upper bounds and therefore unconditionally overwrites the upper-bound on
receiving each fragment. Range tombstones however violate this as a
range tombstone with a smaller position (lower bound) may have a higher
upper bound than some or all fragments that follow it in the stream.
This causes test failures after the converting the combined reader to
v2, but not before, no idea why.
Appending an empty range adjacent to an existing range tombstone would
not deoverlap (by dropping the empty range tombstone) resulting in
different (non canoncial) result depending on the order of appending.
Suppose that range tombstone [a, b] covers range tombstone [x, x), and [a, x) and [x, b) are range tombstones which correspond to [a, b] split around position x.
Appending [a, x) then [x, b) then [x, x) would give [a, b)
Appending [a, x) then [x, x) then [x, b) would give [a, x), [x, x), [x, b)
The fix is to drop empty range tombstones in range_tombstone_list so that the result is canonical.
Fixes#9661Closes#9764
* github.com:scylladb/scylla:
range_tombstone_list: Deoverlap adjacent empty ranges
range_tombstone_list: Convert to work in terms of position_in_partition
Stop using database (and including database.hh) for schema related
purposes and use data_dictionary instead.
data_dictionary::database::real_database() is called from several
places, for these reasons:
- calling yet-to-be-converted code
- callers with a legitimate need to access data (e.g. system_keyspace)
but with the ::database accessor removed from query_processor.
We'll need to find another way to supply system_keyspace with
data access.
- to gain access to the wasm engine for testing whether used
defined functions compile. We'll have to find another way to
do this as well.
The change is a straightforward replacement. One case in
modification_statement had to change a capture, but everything else
was just a search-and-replace.
Some files that lost "database.hh" gained "mutation.hh", which they
previously had access to through "database.hh".
"
These two readers are crucial for writing tests for any composable
reader so we need v2 versions of them before we can convert and test the
combined reader (for example). As these two readers are often used in
situations where the payload they deliver is specially crafted for the
test at hand, we keep their v1 versions too to avoid conversion meddling
with the tests.
Tests: unit(dev)
"
* 'forwarding-and-fragment-reader-v2/v1' of https://github.com/denesb/scylla:
flat_mutation_reader_v2: add make_flat_mutation_reader_from_fragments()
test/lib/mutation_source_test: don't force v1 reader in reverse run
mutation_source: add native_version() getter
flat_mutation_reader_v2: add make_forwardable()
position_in_partition: add after_key(position_in_partition_view)
flat_mutation_reader: make_forwardable(): fix indentation
flat_mutation_reader: make_forwardable(): coroutinize reader
The test in its current form is invalid, as database::remove
does removing the table's name from its listing
as well as from the keyspace metadata, so it won't be found
after that.
That said, database::drop_column_family then proceeds
to truncate and stop the table, after calling await_pending_ops,
and the latter should indeed block on the lock taken by the test.
This change modifies the test to create some sstables in the
table's directory before starting the sstable_directory.
Then, when executing "drop table" in the background,
wait until the table is not found by db.find_column_family
That would fail the test before this change.
See https://jenkins.scylladb.com/job/scylla-enterprise/job/next/1442/artifact/testlog/x86_64_debug/sstable_directory_test.sstable_directory_test_table_lock_works.4720.log
```
INFO 2021-12-13 14:00:17,298 [shard 0] schema_tables - Dropping ks.cf id=00487bc0-5c1d-11ec-9e3b-a44f824027ae version=b10c4994-31c7-3f5a-9591-7fedb0273c82
test/boost/sstable_directory_test.cc(453): fatal error: in "sstable_directory_test_table_lock_works": unexpected exception thrown by table_ok.get()
```
A this point, the test verifies again that the sstables are still on
disk (and no truncate happened), and only after drop completed,
the table should not exist on disk.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20211214104407.2225080-1-bhalevy@scylladb.com>
When the UpdateTable operation is called for a non-existent table, the
appropriate error is ResourceNotFoundException, but before this patch
we ran into an exception, which resulted in an ugly "internal server
error".
In this patch we use the existing get_table() function which most other
operations use, and which does all the appropriate verifications and
generates the appropriate Alternator api_error instead of letting
internal Scylla exceptions escape to the user.
This patch also includes a test for UpdateTable on a non-existent table,
which used to fail before this patch and pass afterwards. We also add a
test for DeleteTable in the same scenario, and see it didn't have this
bug. As usual, both tests pass on DynamoDB, which confirms we generate
the right error codes.
Fixes#9747.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211206181605.1182431-1-nyh@scylladb.com>
The series is on top of "wire up schema raft state machine". It will
apply without, but will not work obviously (when raft is disabled it
does nothing anyway).
This series does not provide any linearisability just yet though. It
only uses raft as a means to distribute schema mutations. To achieve
linearisability more work is needed. We need to at lease make sure
that schema mutation use monotonically increasing timestamps and,
since schema altering statement are RMW, no modification to schema
were done between schema mutation creation and application. If there
were an operation needs to be restarted.
* scylla-dev/gleb/raft-schema-v5: (59 commits)
cql3: cleanup mutation creation code in ALTER TYPE
cql3: use migration_manager::schema_read_barrier() before accessing a schema in altering statements
cql3: bounce schema altering statement to shard 0
migration_manager: add is_raft_enabled() to check if raft is enabled on a cluster
migration_manager: add schema_read_barrier() function
migration_manager: make announce() raft aware
migration_manager: co-routinize announce() function
migration_manager: pass raft_gr to the migration manager
migration_manager: drop view_ptr array from announce_column_family_update()
mm: drop unused announce_ methods
cql3: drop schema_altering_statement::announce_migration()
cql3: drop has_prepare_schema_mutations() from schema altering statement
cql3: drop announce_migration() usage from schema_altering_statement
cql3: move DROP AGGREGATE statement to prepare_schema_mutations() api
migration_manager: add prepare_aggregate_drop_announcement() function
cql3: move DROP FUNCTION statement to prepare_schema_mutations() api
migration_manager: add prepare_function_drop_announcement() function
cql3: move CREATE AGGREGATE statement to prepare_schema_mutations() api
migration_manager: add prepare_new_aggregate_announcement() function
cql3: move CREATE FUNCTION statement to prepare_schema_mutations() api
...
The "ADD" operator in UpdateItem's AttributeUpdates supports a number of
types (numbers, sets and strings), should result in a ValidationException
if the attribute's existing type is different from the type of the
operand - e.g., trying to ADD a number to an attribute which has a set
as a value.
So far we only had partial testing for this (we tested the case where
both operands are sets, but of different types) so this patch adds the
missing tests. The new tests pass (on both Alternator and DynamoDB) -
we don't have a bug there.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211213195023.1415248-1-nyh@scylladb.com>
Appending an empty range adjacent to an existing range tombstone would
not deoverlap (by dropping the empty range tombstone) resulting in
different (non canoncial) result depending on the order of appending.
Suppose that [a, b] covers [x, x)
Appending [a, x) then [x, b) then [x, x) would give [a, b)
Appending [a, x) then [x, x) then [x, b) would give [a, x), [x, x), [x, b)
Fix by dropping empty range tombstones.
Once current window is sealed, TWCS is supposed to compact all its
sstables into one. If there's ongoing compaction, it can happen that
sstables are missed and therefore past windows will contain more than
one sstable. Additionally, it could happen that major doesn't happen
at all if under heavy load. All these problems are fixed by serializing
major on past window and also postponing it if manager refuses to run
the job now.
Fixes#9553.
Reviewed-by: Benny Halevy <bhalevy@scylladb.com>
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
STCS option can be retrieved from class member, as newest_bucket()
is no longer a static function. let's get rid of it.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
The code in test/cql-pytest/run.py can start Scylla (or Cassandra, or
Redis, etc.) in a random IP address in 127.*.*.*. We explained in a
comment that 127.0.0.* is used by CCM so we avoid it in case someone
runs both dtest and test.py in parallel on the same machine.
But this observation was not accurate: Although the original CCM did use
only 127.0.0.*, in Scylla's CCM we added in 2017, in commit
00d3ba5562567ab83190dd4580654232f4590962, the ability to run multiple
copies of CCM in parallel; CCM now uses 127.0.*.*, not just 127.0.0.*.
So we need to correct this in the comment.
Luckily, the code doesn't need to change! We already avoided the entire
127.0.*.* for simplicity, not just 127.0.0.*.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211212151339.1361451-1-nyh@scylladb.com>
"
Today, data from different buckets (e.g. windows) cannot be compacted together because
mutation compactor happens inside each consumer, where each consumer is done on behalf
of a particular bucket. To solve this problem, mutation compaction process is being
moved from consumer into producer, such that interposer consumer, which is responsible
for segregation, will be feeded with compacted data and forward it into the owner bucket.
Fixes#9662.
tests: unit(debug).
"
* 'compact_across_buckets_v2' of github.com:raphaelsc/scylla:
tests: sstable_compaction_test: add test_twcs_compaction_across_buckets
compaction: Move mutation compaction into producer for TWCS
compaction: make enable_garbage_collected_sstable_writer() more precise
Verify that TWCS compaction can now compact data across time windows,
like a tombstone which will cause all shadowed data to be purged
once they're all compacted together.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
The main difference compared to v1 (apart from having _v2 suffix at
relevant places) is how slicing and reversing works. The v2 variant has
native reverse support built-in because the reversing reader is not
something we want to convert to v2.
A native v2 mutation-source test is also added.
Currently in the reverse run we wrap the test-provided mutation-source
and create a v1 reader with it, forcing a conversion if the
mutation-source has a v2 factory. Worse still, if the test is v2 native,
there will be a double conversion. This patch fixes this by creating a
wrapper mutation-source appropriate to the version of the underlying
factory of the wrapped mutation-source.
There are several places that (still) use throwing b-tree .insert_before()
method and don't manage the inserted object lifetime. Some of those places
also leave the leaked rows_entry on the LRU delaying the assertion failure
by the time those entries get evicted (#9728)
To prevent such surprises in the future, the set removes the non-safe
inserters from the B-tree code. Actually most of this set is that removal
plus preparations for reviewability.
* xemul/br-rows-insertion-exception-safety-2:
btree: Earnestly discourage from insertion of plain references
row-cache: Handle exception (un)safety of rows_entry insertion
partition_snapshot_row_cursor: Shuffle ensure_result creation
mutation_partition: Use B-tree insertion sugar
tests: Make B-tree tests use unique-ptrs for insertion
In issue #9406 we noticed that a counter for BatchGetItem operations
was missing. When we fixed it, we added a test which checked this
counter - but only this counter. It was left as a TODO to test the rest
of the Alternator metrics, and this is what this patch does.
Here we add a comprehensive test for *all* of the operations supported
by Scylla and how they increase the appropriate operation counter.
With this test we discovered a new bug: the DescribeTimeToLive operation
incremented the UpdateTimeToLiveCounter :-( So in this patch we also
include a fix for that bug, and the new test verifies that it is fixed.
In addition to the operation counters, Alternator also has additional
metric and we also added tests for some of them - but not all. The
remaining untested metrics are listed in a TODO comment.
Message-Id: <20211206154727.1170112-1-nyh@scylladb.com>
do_with_some_data runs a function in a seastar thread.
It needs to get() the future func returns rather
than propagating it.
This solves a secondary failure due to abandoned future
when the test case fails, as seen in
https://jenkins.scylladb.com/view/master/job/scylla-master/job/next/4254/artifact/testlog/x86_64_debug/database_test.snapshot_with_quarantine_works.381.log
```
test/boost/database_test.cc(903): fatal error: in "snapshot_with_quarantine_works": critical check expected.empty() has failed
WARN 2021-12-08 00:35:16,300 [shard 0] seastar - Exceptional future ignored: boost::execution_aborted, backtrace: 0x10935e50 0x16ff2d8d 0x16ff2a4d 0x16ff5033 0x16ff5ec2 0x162d4ce9 0x10a2bdb5 0x10a2bd24 0x10a54ca4 0x10a27cf3 0x10a22151 0x10a67c9d 0x10a67a78 0x163ac37e 0x163b29e9 0x163b7690 0x163b51c1 0x17c212df 0x17c1f097 0x17bf8b4c 0x17bf83f2 0x17bf82a2 0x17bf7d52 0x10f8bf5a 0x166db84b /lib64/libpthread.so.0+0x9298 /lib64/libc.so.6+0x100352
...
*** 1 abandoned failed future(s) detected
Failing the test because fail was requested by --fail-on-abandoned-failed-futures
```
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20211209174512.851945-1-bhalevy@scylladb.com>
In queries like:
```cql
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c1 ASC, c2 ASC)
```
we can skip the requirement to specify ordering for `c1` column.
The `c1` column is restricted by an `EQ` restriction, so it can have
at most one value anyway, there is no need to sort.
This commit makes it possible to write just:
```cql
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c2 ASC)
```
I reorganized the ordering code, I feel that it's now clearer and easier to understand.
It's possible to only introduce a small change to the existing code, but I feel like it becomes a bit too messy.
I tried it out on the [`orderby_disorder_small`](https://github.com/cvybhu/scylla/commits/orderby_disorder_small) branch.
The diff is a bit messy because I moved all ordering functions to one place,
it's better to read [select_statement.cc](https://github.com/cvybhu/scylla/blob/orderby_disorder/cql3/statements/select_statement.cc#L1495-L1658) lines 1495-1658 directly.
In the new code it would also be trivial to allow specifying columns in any order, we would just have to sort them.
For now I commented out the code needed to do that, because the point of this PR was to fix#2247.
Allowing this would require some more work changing the existing tests.
Fixes: #2247Closes#9518
* github.com:scylladb/scylla:
cql-pytest: Enable test for skipping eq restricted columns in order by
cql3: Allow to skip EQ restricted columns in ORDER BY
cql3: Add has_eq_restriction_on_column function
cql3: Reorganize orderings code
This test was marked as xfail, but now the functionality it tests has been implemented.
In my opinion the expected error message makes no sense, the message was:
"Order by currently only supports the ordering of columns following their declared order in the PRIMARY KEY"
In cases where there was missing restriction on one column.
This has been changed to:
"Unsupported order by relation - column {} doesn't have an ordering or EQ relation."
Because of that I had to modify the test to accept messages from both Scylla and Cassandra.
The expected error message pattern is now "rder by", because that's the largest common part.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Allow stopping compaction by type on a given keyspace
and list of tables.
Add respective rest_api test.
Fixes#9700
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Mutations are not guaranteed to come in the order of their timestamps.
If there is an expired tombstone in the sstable and a repair inserts old
data into memtable, the compaction would not consider memtable data and
purge the tombstone leading to data resurrection. The solution is to
disallow purging tombstones newer than min memtable timestamp.
