PR #9314 fixed a similar issue with regular insert statements
but missed the LWT code path.
It's expected behaviour of
modification_statement::create_clustering_ranges to return an
empty range in this case, since possible_lhs_values it
uses explicitly returns empty_value_set if it evaluates rhs
to null, and it has a comment about it (All NULL
comparisons fail; no column values match.) On the other hand,
all components of the primary key are required to be set,
this is checked at the prepare phase, in
modification_statement::process_where_clause. So the only
problem was modification_statement::execute_with_condition
was not expecting an empty clustering_range in case of
a null clustering key.
Fixes: #11954
Take advantage of the facts that both the owned ranges
and the initial non_owned_ranges (derived from the set of sstables)
are deoverlapped and sorted by start token to turn
the calculation of the final non_owned_ranges from
quadratic to linear.
Fixes#11922Closes#11903
* github.com:scylladb/scylladb:
dht: optimize subtract_ranges
compaction: refactor dht::subtract_ranges out of get_ranges_for_invalidation
compaction_manager: needs_cleanup: get first/last tokens from sstable decorated keys
In order to support different storage kinds for sstable files (e.g. -- s3) it's needed to localize all the places that manipulate files on a POSIX filesystem so that custom storage could implement them in its own way. This set moves the deletion log manipulations to the sstable_directory.cc, which already "knows" that it works over a directory.
Closes#12020
* github.com:scylladb/scylladb:
sstables: Delete log file in replay_pending_delete_log()
sstables: Move deletion log manipulations to sstable_directory.cc
sstables: Open-code delete_sstables() call
sstables: Use fs::path in replay_pending_delete_log()
sstables: Indentation fix after previous patch
sstables: Coroutinize replay_pending_delete_log
sstables: Read pending delete log with one line helper
sstables: Dont write pending log with file_writer
The algorithm is generic and can be used elsewhere.
Add a unit test for the function before it gets
optimized in the following patch.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
The deletion log concept uses the fact that files are on a POSIX
filesystem. Support for another storage type will have to reimplement
this place, so keep the FS-specific code in _directory.cc file.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
We had an xfailing test that reproduced a case where Alternator tried
to report an error when the request was too long, but the boto library
didn't see this error and threw a "Broken Pipe" error instead. It turns
out that this wasn't a Scylla bug but rather a bug in urllib3, which
overzealously reported a "Broken Pipe" instead of trying to read the
server's response. It turns out this issue was already fixed in
https://github.com/urllib3/urllib3/pull/1524
and now, on modern installations, the test that used to fail now passes
and reports "XPASS".
So in this patch we remove the "xfail" tag, and skip the test if
running an old version of urllib3.
Fixes#8195Closes#12038
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
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>
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
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
`get_rpc_client` calculates a `topology_ignored` field when creating a
client which says whether the client's endpoint had topology information
when this client was created. This is later used to check if that client
needs to be dropped and replaced with a new client which uses the
correct topology information.
The `topology_ignored` field was incorrectly calculated as `true` for
pending endpoints even though we had topology information for them. This
would lead to unnecessary drops of RPC clients later. Fix this.
Remove the default parameter for `with_pending` from
`topology::has_endpoint` to avoid similar bugs in the future.
Apparently this fixes#11780. The verbs used by decommission operation
use RPC client index 1 (see `do_get_rpc_client_idx` in
message/messaging_service.cc). From local testing with additional
logging I found that by the time this client is created (i.e. the first
verb in this group is used), we already know the topology. The node is
pending at that point - hence the bug would cause us to assume we don't
know the topology, leading us to dropping the RPC client later, possibly
in the middle of a decommission operation.
Fixes: #11780Closes#11942
* github.com:scylladb/scylladb:
message: messaging_service: check for known topology before calling is_same_dc/rack
test: reenable test_topology::test_decommission_node_add_column
test/pylib: util: configurable period in wait_for
message: messaging_service: fix topology_ignored for pending endpoints in get_rpc_client
message: messaging_service: topology independent connection settings for GOSSIP verbs
It's interesting that prepare_expression
for column identifiers doesn't require a receiver.
I hope this won't break validation in the future.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Add a function which creates a mock instance
of data_dictionary::database.
prepare_expression requires a data_dictionary::database
as an argument, so unit tests for it need something
to pass there. make_data_dictionary_database can
be used to create an instance that is sufficient for tests.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
This type is currently an unordered_set, but only consists of at most
two elements. Making it an enum_set renders it into a size_t variable
and better describes the intention.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
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).
Pass a change diff into the notification callback,
rather than add or remove servers one by one, so that
if we need to persist the state, we can do it once per
configuration change, not for every added or removed server.
For now still pass added and removed entries in two separate calls
per a single configuration change. This is done mainly to fulfill the
library contract that it never sends messages to servers
outside the current configuration. The group0 RPC
implementation doesn't need the two calls, since it simply
marks the removed servers as expired: they are not removed immediately
anyway, and messages can still be delivered to them.
However, there may be test/mock implementations of RPC which
could benefit from this contract, so we decided to keep it.
This patch adds a reproducer for issue #11954: Attempting an
"IF NOT EXISTS" (LWT) write with a null key crashes Scylla,
instead of producing a simple error message (like happens
without the "IF NOT EXISTS" after #7852 was fixed).
The test passed on Cassandra, but crashes Scylla. Because of this
crash, we can't just mark the test "xfail" and it's temporarily
marked "skip" instead.
Refs #11954.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#11982
As P. T. Barnoom famously said, "write what you like but spell my name
correctly". Following that, we correct the spelling of Barrett's name
in the source tree.
Closes#11989
Also improve the test to increase the probability of reproducing #11780
by injecting sleeps in appropriate places.
Without the fix for #11780 from the earlier commit, the test reproduces
the issue in roughly half of all runs in dev build on my laptop.
When we write to a materialized view, we need to know some information
defined in the base table such as the columns in its schema. We have
a "view_info" object that tracks each view and its base.
This view_info object has a couple of mutable attributes which are
used to lazily-calculate and cache the SELECT statement needed to
read from the base table. If the base-table schema ever changes -
and the code calls set_base_info() at that point - we need to forget
this cached statement. If we don't (as before this patch), the SELECT
will use the wrong schema and writes will no longer work.
This patch also includes a reproducing test that failed before this
patch, and passes afterwords. The test creates a base table with a
view that has a non-trivial SELECT (it has a filter on one of the
base-regular columns), makes a benign modification to the base table
(just a silly addition of a comment), and then tries to write to the
view - and before this patch it fails.
Fixes#10026Fixes#11542
As indicated in #11816, we'd like to enable deserializing vectors in reverse.
The forward deserialization is achieved by reading from an input_stream. The
input stream internally is a singly linked list with complicated logic. In order to
allow for going through it in reverse, instead when creating the reverse vector
initializer, we scan the stream and store substreams to all the places that are a
starting point for a next element. The iterator itself just deserializes elements
from the remembered substreams, this time in reverse.
Fixes#11816Closes#11956
* github.com:scylladb/scylladb:
test/boost/serialization_test.cc: add test for reverse vector deserializer
serializer_impl.hh: add reverse vector serializer
serializer_impl: remove unneeded generic parameter
As noted in issue #11979, Scylla inconsistently (and unlike Cassandra)
requires "IS NOT NULL" one some but not all materialized-view key
columns. Specifically, Scylla does not require "IS NOT NULL" on the
base's partition key, while Cassandra does.
This patch is a test which demonstrates this inconsistency. It currently
passes on Cassandra and fails on Scylla, so is marked xfail.
Refs #11979
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closes#11980
The helper is already widely used, one (last) test case can benefit from using it too
Closes#11978
* github.com:scylladb/scylladb:
test: Indentation fix after previous patch
test: Wse with_sstable_directory() helper
