There is a more general version of limits()
which takes expressions as both the lhs and rhs
arguments.
There is no need for a specialized overload.
This specialized overload takes a tuple_constructor
as lhs, but we call evaluate() on both sides
of a binary operator before checking equality,
so this won't be useful at all.
Having multiple functions increases the risk
that one of them has a bug, while giving
dubious benfit.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Expressions like:
123 = NULL
NULL = 123
NULL = NULL
NULL != 123
should be tolerated, but evaluate to NULL.
The current code assumes that a binary operator
can only evaluate to a boolean - true or false.
Now a binary operator can also evaluate to NULL.
This should happen in cases when one of the
operator's sides is NULL.
A special class is introduced to represent a value
that can be one of three things: true, false or null.
It's better than using std::optional<bool>,
because optional has implicit conversions to bool
that could cause confusion and bugs.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
There is a more general version of equal()
which takes expressions as both the lhs and rhs
arguments.
There is no need for a specialized overload.
This specialized overload takes a tuple_constructor
as lhs, but we call evaluate() on both sides
of a binary operator before checking equality,
so this won't be useful at all.
Having multiple functions increases the risk
that one of them has a bug, while giving
dubious benfit.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
is_satisfied_by has to check if a binary_operator is satisfied
by some values. It used to be impossible to evaluate
a binary_operator, so is_satisfied had code to check
if its satisfied for a limited number of cases
occuring when filtering queries.
Now evaluate(binary_operator) has been implemented
and is_satisfied_by can use it to check if a binary_operator
evaluates to true.
This is cleaner and reduces code duplication.
Additionally cql tests will test the new evalute() implementation.
There is one special case with token().
When is_satisfied_by sees a restriction on token
it assumes that it's satisfied because it's
sure that these token restrictions were used
to generate partition ranges.
I had to leave this special case in because it's impossible
to evaluate(token). Once this is implemented I will remove
the special case because it's risky and prone to cause
bugs.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
The code to evaluate binary operators
was copied from is_satisfied_by.
is_satisfied_by wasn't able to evaluate
IS NOT NULL restrictions, so when such restriction
is encountered it throws an exception.
Implement proper handling for IS NOT NULL binary operators.
The switch ensures that all variants of oper_t are handled,
otherwise there would be a compilation error.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
evaluate() takes an expression and evaluates it
to a constant value. It wasn't possible to evalute
binary operators before, so it's added.
The code is based on is_satisfied_by,
which is currently used to check
whether a binary operator evaluates
to true or false.
It looks like is_satisfied_by and evalate()
do pretty much the same thing, one could be
implemented using the other.
In the future they might get merged
into a single function.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
like() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
contains_key() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
contains() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Fragment reordering and fragment dropping bugs have been plaguing us since forever. To fight them we added a validator to the sstable write path to prevent really messed up sstables from being written.
This series adds validation to the mutation compactor. This will cover reads and compaction among others, hopefully ridding us of such bugs on the read path too.
This series fixes some benign looking issues found by unit tests after the validator was added -- although how benign a producer emitting two partition-ends depends entirely on how the consumer reacts to it, so no such bug is actually benign.
Fixes: https://github.com/scylladb/scylladb/issues/11174Closes#11532
* github.com:scylladb/scylladb:
mutation_compactor: add validator
mutation_fragment_stream_validator: add a 'none' validation level
test/boost/mutation_query_test: test_partition_limit: sort input data
querier: consume_page(): use partition_start as the sentinel value
treewide: use ::for_partition_end() instead of ::end_of_partition_tag_t{}
treewide: use ::for_partition_start() instead of ::partition_start_tag_t{}
position_in_partition: add for_partition_{start,end}()
Adds unit tests for the function `expr::prepare_expression`.
Three minor bugs were found by these tests, both fixed in this PR.
1. When preparing a map, the type for tuple constructor was taken from an unprepared tuple, which has `nullptr` as its type.
2. Preparing an empty nonfrozen list or set resulted in `null`, but preparing a map didn't. Fixed this inconsistency.
3. Preparing a `bind_variable` with `nullptr` receiver was allowed. The `bind_variable` ended up with a `nullptr` type, which is incorrect. Changed it to throw an exception,
Closes#11941
* github.com:scylladb/scylladb:
test preparing expr::usertype_constructor
expr_test: test that prepare_expression checks style_type of collection_constructor
expr_test: test preparing expr::collection_constructor for map
prepare_expr: make preparing nonfrozen empty maps return null
prepare_expr: fix a bug in map_prepare_expression
expr_test: test preparing expr::collection_constructor for set
expr_test: test preparing expr::collection_constructor for list
expr_test: test preparing expr::tuple_constructor
expr_test: test preparing expr::untyped_constant
expr_test_utils: add make_bigint_raw/const
expr_test_utils: add make_tinyint_raw/const
expr_test: test preparing expr::bind_variable
cql3: prepare_expr: forbid preparing bind_variable without a receiver
expr_test: test preparing expr::null
expr_test: test preparing expr::cast
expr_test_utils: add make_receiver
expr_test_utils: add make_smallint_raw/const
expr_test: test preparing expr::token
expr_test: test preparing expr::subscript
expr_test: test preparing expr::column_value
expr_test: test preparing expr::unresolved_identifier
expr_test_utils: mock data_dictionary::database
We had a test that used to fail because of issue #8745. But this issue
was alread fixed, and we forgot to remove the "xfail" marker. The test
now passes, so let's remove the xfail marker.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#12039
Add new guide for upgrading 5.1 to 5.2.
In this new upgrade doc, include additional steps for enabling
Raft using the `consistent_cluster_management` flag. Note that we don't
have this flag yet but it's planned to replace the experimental flag in
5.2.
In the "Raft in ScyllaDB" document, add sections about:
- enabling Raft in existing clusters in Scylla 5.2,
- verifying that the internal Raft upgrade procedure finishes
successfully,
- recovering from a stuck Raft upgrade procedure or from a majority loss
situation.
Fix some problems in the documentation, e.g. it is not possible to
enable Raft in an existing cluster in 5.0, but the documentation claimed
that it is.
Follow-up items:
- if we decide for a different name for `consistent_cluster_management`,
use that name in the docs instead
- update the warnings in Scylla to link to the Raft doc
- mention Enterprise versions once we know the numbers
- update the appropriate upgrade docs for Enterprise versions
once they exist
Closes#11910
* github.com:scylladb/scylladb:
docs: describe the Raft upgrade and recovery procedures
docs: add upgrade guide 5.1 -> 5.2
We recently (in 7fbad8de87) made sure all admission paths can trigger the eviction of inactive reads. As reader eviction happens in the background, a mechanism was added to make sure only a single eviction fiber was running at any given time. This mechanism however had a preemption point between stopping the fiber and releasing the evict lock. This gave an opportunity for either new waiters or inactive readers to be added, without the fiber acting on it. Since it still held onto the lock, it also prevented from other eviction fibers to start. This could create a situation where the semaphore could admit new reads by evicting inactive ones, but it still has waiters. Since an empty waitlist is also an admission criteria, once one waiter is wrongly added, many more can accumulate.
This series fixes this by ensuring the lock is released in the instant the fiber decides there is no more work to do.
It also fixes the assert failure on recursive eviction and adds a detection to the inactive/waiter contradiction.
Fixes: #11923
Refs: #11770Closes#12026
* github.com:scylladb/scylladb:
reader_concurrency_semaphore: do_wait_admission(): detect admission-waiter anomaly
reader_concurrency_semaphore: evict_readers_in_the_background(): eliminate blind spot
reader_concurrency_semaphore: do_detach_inactive_read(): do a complete detach
This series contains a mixed bag of improvements to `scylla sstable dump-data`. These improvements are mostly aimed at making the json output clearer, getting rid of any ambiguities.
Closes#12030
* github.com:scylladb/scylladb:
tools/scylla-sstable: traverse sstables in argument order
tools/scylla-sstable: dump-data docs: s/clustering_fragments/clustering_elements
tools/scylla-sstable: dump-data/json: use Null instead of "<unknown>"
tools/scylla-sstable: dump-data/json: use more uniform format for collections
tools/scylla-sstable: dump-data/json: make cells easier to parse
Since recently the framework uses a separate set of unique IDs to
identify servers, but the log file and workdir is still named using the
last part of the IP address.
This is confusing: the test logs sometimes don't provide the IP addr
(only the ID), and even if they do, the reader of the test log may not
know that they need to look at the last part of the IP to find the
node's log/workdir.
Also using ID will be necessary if we want to reuse IP addresses (e.g.
during node replace, or simply not to run out of IP addresses during
testing).
So use the ID instead to name the workdir and log file.
Also, when starting a test case, print the used cluster. This will make
it easier to map server IDs to their IP addresses when browsing through
the test logs.
Closes#12018
* github.com:scylladb/scylladb:
test/pylib: manager_client: print used cluster when starting test case
test/pylib: scylla_cluster: use server ID to name workdir and log file, not IP address
BOOST_CHECK_EQUAL is a weaker form of assertion, it reports an error
and will cause the test case to fail but continues. This makes the
test harder to debug because there's no obvious way to catch the
failure in GDB and the test output is also flooded with things which
happen after the failed assertion.
Message-Id: <20221119171855.2240225-1-tgrabiec@scylladb.com>
When filtering with multi column restriction present all other restrictions were ignored.
So a query like:
`SELECT * FROM WHERE pk = 0 AND (ck1, ck2) < (0, 0) AND regular_col = 0 ALLOW FILTERING;`
would ignore the restriction `regular_col = 0`.
This was caused by a bug in the filtering code:
2779a171fc/cql3/selection/selection.cc (L433-L449)
When multi column restrictions were detected, the code checked if they are satisfied and returned immediately.
This is fixed by returning only when these restrictions are not satisfied. When they are satisfied the other restrictions are checked as well to ensure all of them are satisfied.
This code was introduced back in 2019, when fixing #3574.
Perhaps back then it was impossible to mix multi column and regular columns and this approach was correct.
Fixes: #6200Fixes: #12014Closes#12031
* github.com:scylladb/scylladb:
cql-pytest: add a reproducer for #12014, verify that filtering multi column and regular restrictions works
boost/restrictions-test: uncomment part of the test that passes now
cql-pytest: enable test for filtering combined multi column and regular column restrictions
cql3: don't ignore other restrictions when a multi column restriction is present during filtering
In issue #12014 a user has encountered an instance of #6200.
When filtering a WHERE clause which contained
both multi-column and regular restrictions,
the regular restrictions were ignored.
Add a test which reproduces the issue
using a reproducer provided by the user.
This problem is tested in another similar test,
but this one reproduces the issue in the exact
way it was found by the user.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
A part of the test was commented out due to #6200.
Now #6200 has been fixed and it can be uncommented.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
The test test_multi_column_restrictions_and_filtering was marked as xfail,
because issue #6200 wasn't fixed. Now that filtering
multi column and other restrictions together has been fixed
the test passes.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
When filtering with multi column restriction present all other restrictions were ignored.
So a query like:
`SELECT * FROM WHERE pk = 0 AND (ck1, ck2) < (0, 0) AND regular_col = 0 ALLOW FILTERING;`
would ignore the restriction `regular_col = 0`.
This was caused by a bug in the filtering code:
2779a171fc/cql3/selection/selection.cc (L433-L449)
When multi column restrictions were detected,
the code checked if they are satisfied and returned immediately.
This is fixed by returning only when these restrictions
are not satisfied. When they are satisfied the other
restrictions are checked as well to ensure all
of them are satisfied.
This code was introduced back in 2019, when fixing #3574.
Perhaps back then it was impossible to mix multi column
and regular columns and this approach was correct.
Fixes: #6200Fixes: #12014
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
The currently used "<unknown>" marker for invalid values/types is
undistinguishable from a normal value in some cases. Use the much more
distinct and unique json Null instead.
Instead of trying to be clever and switching the output on the type of
collection, use the same format always: a list of objects, where the
object has a key and value attribute, containing to the respective
collection item key and values. This makes processing much easier for
machines (and humans too since the previous system wasn't working well).
There are several slightly different cell types in scylla: regular
cells, collection cells (frozen and non-frozen) and counter cells
(update and shards). In C++ code the type of the cell is always
available for code wishing to make out exactly what kind of cell a cell
is. In the JSON output of the dump-data this is currently really hard to
do as there is not enough information to disambiguate all the different
cell types. We wish to make the JSON output self-sufficient so in this
patch we introduce a "type" field which contains one of:
* regular
* counter-update
* counter-shards
* frozen-collection
* collection
Furthermore, we bring the different types closer by also printing the
counter shards under the 'value' key, not under the 'shards' key as
before. The separate 'shards' is no longer needed to disambiguate.
The documentation and the write operation is also updated to reflect the
changes.
Some tests may take longer than a few seconds to run. We want to
mark such tests in some way, so that we can run them selectively.
This patch proposes to use pytest markers for this. The markers
from the test.py command line are passed to pytest
as is via the -m parameter.
By default, the marker filter is not applied and all tests
will be run without exception. To exclude e.g. slow tests
you can write --markers 'not slow'.
The --markers parameter is currently only supported
by Python tests, other tests ignore it. We intend to
support this parameter for other types of tests in the future.
Another possible improvement is not to run suites for which
all tests have been filtered out by markers. The markers are
currently handled by pytest, which means that the logic in
test.py (e.g., running a scylla test cluster) will be run
for such suites.
Closes#11713
This reverts commit 22f13e7ca3, and reinstates
commit df8e1da8b2 ("Merge 'cql3: select_statement:
coroutinize indexed_table_select_statement::do_execute_base_query()' from
Avi Kivity"). The original commit was reverted due to failures in debug
mode on aarch64, but after commit 224a2877b9
("build: disable -Og in debug mode to avoid coroutine asan breakage"), it
works again.
Closes#12021
We plan to stop storing IP addresses in Raft configuration, and instead
use the information disseminated through gossip to locate Raft peers.
Implement patches that are building up to that:
* improve Raft API of configuration change notifications
* disseminate raft host id in Gossip
* avoid using Raft addresses from Raft configuraiton, and instead
consistently use the translation layer between raft server id <-> IP
address
Closes#11953
* github.com:scylladb/scylladb:
raft: persist the initial raft address map
raft: (upgrade) do not use IP addresses from Raft config
raft: (and gossip) begin gossiping raft server ids
raft: change the API of conf change notifications
The lister accepts sort of a filter -- what kind of entries to list, regular, directories or both. It currently uses unordered_set, but enum_set is shorter and better describes the intent.
Closes#12017
* github.com:scylladb/scylladb:
lister: Make lister::dir_entry_types an enum_set
database: Avoid useless local variable
The semaphore should admit readers as soon as it can. So at any point in
time there should be either no waiters, or the semaphore shouldn't be
able to admit new reads. Otherwise something went wrong. Detect this
when queuing up reads and dump the diagnostics if detected.
Even though tests should ensure this should never happen, recently we've
seen a race between eviction and enqueuing producing such situations.
This is very hard to write tests for, so add built-in detection and
protection instead. Detecting this is very cheap anyway.
Said method has a protection against concurrent (recursive more like)
calls to itself, by setting a flag `_evicting` and returning early if
this flag is set. The evicting loop however has at least one preemption
point between deciding there is nothing more to evict and resetting said
flag. This window provides opporunity for new inactive reads or waiters
to be queued without this loop noticing, while denying any other
concurrent invocations at that time from reacting too.
Eliminate this by using repeat() instead of do_until() and setting
`_evicting = false` the moment the loop's run condition becomes false.
Currently this method detaches the inactive read from the handle and
notifies the permit, calls the notify handler if any and does some stat
bookkeeping. Extend it to do a complete detach: unlink the entry from
the inactive reads list and also cancel the ttl timer.
After this, all that is left to the caller is to destroy the entry.
This will prevent any recursive eviction from causing assertion failure.
Although recursive eviction shouldn't happen, it shouldn't trigger an
assert.
Since commit a980f94 (token_metadata: impl: keep the set of normal token owners as a member), we have a set, _normal_token_owners, which contains all the nodes in the ring.
We can use _normal_token_owners to check if a node is part of the ring directly instead of going through the _toplogy indirectly.
Fixes#11935Closes#11936
* github.com:scylladb/scylladb:
token_metadata: Rename is_member to is_normal_token_owner
token_metadata: Add docs for is_member
token_metadata: Do not use topology info for is_member check
token_metadata: Check node is part of the topology instead of the ring
Since commit a980f94 (token_metadata: impl: keep the set of normal token
owners as a member), we have a set, _normal_token_owners, which contains
all the nodes in the ring.
We can use _normal_token_owners to check if a node is part of the ring
directly instead of going through the _toplogy indirectly.
Fixes#11935
update_normal_tokens is the way to add a new node into the ring. We
should not require a new node to already be in the ring to be able to
add it to the ring. The current code works accidentally because
is_member is checking if a node is in the topology
We should use _topology.has_endpoint to check if a node is part of the
topology explicitly.
In Scylla and Cassandra inserting an empty collection
that is not frozen, is interpreted as inserting a null value.
list_prepare_expression and set_prepare_expression
have an if which handles this behavior, but there
wasn't one in map_prepare_expression.
As a result preparing empty list or set would result in null,
but preparing an empty map wouldn't. This is inconsistent,
it's better to return null in all cases of empty nonfrozen
collections.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
map_prepare_expression takes a collection_constructor
of unprepared items and prepares it.
Elements of a map collection_constructor are tuples (key and value).
map_prepare_expression creates a prepared collection_constructor
by preparing each tuple and adding it to the result.
During this preparation it needs to set the type of the tuple.
There was a bug here - it took the type from unprepared
tuple_constructor and assigned it to the prepared one.
An unprepared tuple_constructor doesn't have a type
so it ended up assigning nullptr.
Instead of that it should create a tuple_type_impl instance
by looking at the types of map key and values,
and use this tuple_type_impl as the type of the prepared tuples.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
